TUNTAP_MODE_PERSIST broken?

[oe1rsa]

I tried to create a persistent tun0 on linux, but failed. While the setting of the persistence works ok, the logic in the file tuntap-unix-linux.c looks broken:

void
tuntap_sys_destroy(struct device *dev)
{
       if (ioctl(dev->tun_fd, TUNSETPERSIST, 0) == -1) {
               tuntap_log(TUNTAP_LOG_WARN, "Can't destroy the interface");
       }
}

This function tries to unconditionally set the persistence to "do not persist" and only if the call fails logs a message which has not much to do with persistence.

To me it looks as if the author of the destroy function might have been in the believe that the 0 argument makes the ioctl read the current state. This is, however, not the case. The effect is that the persistence is cleared and it is impossible with libtuntap to create a persistent interface.

[tleguern]

In the regress folder you can find the file helper34.c which implements a persistent tun0 device and then destroys it.
Putting a simple sleep(10); before the call to tuntap_destroy shows that it works well, at least on Ubuntu 24.04 :

$ ip link show tun0
Device "tun0" does not exist.
$ sudo ./helper34 &
[3] 486848
$ ip link show tun0
14: tun0: <POINTOPOINT,MULTICAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 500
    link/none 
$ 
[3]   Done                 \sudo ./helper34
$ ip link show tun0
Device "tun0" does not exist.

The function tuntap_sys_destroy you quoted uses the ioctl flag TUNSETPERSIST to remove the persistance bit, that seems to be the correct job.

[tleguern]

The code using TUNSETPERSIST to set the persistance bit and thus creating a persistent interface is line 103 of tuntap-unix-linux.c in the function tuntap_sys_start().

[oe1rsa]

Maybe I do not understand libtuntaps idea of persistence. What I know about persistence is, that when the process has gone, the interface is still there and persists.

[tleguern]

Looks like we have the same understanding yes. If you want a persistant interface call it with TUNTAP_MODE_PERSIST and don't call tuntap_destroy later.

[oe1rsa]

Ah, yes. This might be the problem: I am trying to use the C++ interface, which forces to run destroy on destruction of the wrapper object. 🤔

[chipot]

I think the rationale is that if your process goes down then the routine that reads from the tap stops and, for a vpn-like application that would mean no connectivity anymore. So it seemed reasonable to destroy the interface. Now in hindsight it might be super disruptive to destroy the interface if the attached process just needer a quick restart and will reattach soonish.
I guess I would be OK with a PR to change that behavior, but can you elaborate on your use case first?

[oe1rsa]

@chipot: Whatever rationale there might be, I prefer policy to be set by the user of the library. The current situation is, that even that a user specifies PERSIST, she will be seeing it as a bug since 1) the tun/tap does not persist and 2) there is no error message.

To be fair, however, using the C++ interface it was not possible to request persistency at all. (Before I introduced my last change.) But then: The C++ binding will force persistence policy over the C interface, which in my humble opinion is not a good thing.

Since libtuntap, as I see its value, is abstracting away platform differences, of course there should be some agreement on the common set of supported features. Persistence, e.g. behaves different and the current implementation is to signal an error (or warning) on the windows platform. It would be possible to not touch persistence in libtuntap at all and delegate persistence to the administrator. The persistence is a property that can be set on the interface outside libtuntap.

My preference would be: obey the C interface. However I would suggest to better document the slight difference of the calls tuntap_destroy and tuntap_release. In particular one should call only either one, but not both and also the order matters, e.g. if calling tuntap_release (freeing the memory for dev) and then tuntap_destroy will trigger undefined behaviour, since already freed memory is accessed.

[oe1rsa]

@tleguern

Looks like we have the same understanding yes. If you want a persistant interface call it with TUNTAP_MODE_PERSIST and don't call tuntap_destroy later.

But, then one should call tuntap_release instead to avoid a memory leak, yes?

[tleguern]

But, then one should call tuntap_release instead to avoid a memory leak, yes?

Yes, as well as avoiding file descriptor leak. It is described (succinctly) in the file doc/api.md.

[oe1rsa]

Ok, thank you. Now that I know that "leaving the interface" is related to TUNTAP_MODE_PERSIST I do understand the documentation. Still, since I didn't that get straight on first reading, possibly the documentation should mention it a bit more prominently. At least the author of the cpp binding didn't this get right in the first place as well. (Well, to be fair, possibly he didn't want to bind the persistence mode. But if so she should have called tuntap_release rather than tuntap_destroy.)

Btw. any chance getting my pull request reviewed? I mean I am happy with a reject as well if you think the change isn't wanted. I just need to steer my future directions.

[tleguern]

Btw. any chance getting my pull request reviewed?

Please wait a bit. We have work and family to handle too :)

[oe1rsa]

Ok, fine. I didn't want to sound pushy. First things first. 👶