if_tuntap: make SIOCIFDESTROY interruptible

[kevans]

Summary

There's no good justification to permanently hang a thread until the
tunnel can be destroyed. Make it interruptible so that the admin can
^C it and remedy the situation if something erroneously has the tunnel
open, rather than forcing them to open another shell to resolve it.

[markj]

sys/net/if_tuntap.c

632–633

Hmm, shouldn't this be a while loop?

637

I would just use cv_wait() here so that the locking protocol is consistent. Then we can unconditionally unlock below instead of having the tp->tun_busy == 0 || may_intr check.

[zlei]

Shall we firstly signal the application that the tuntap device is going to be shutdown and destroyed and secondly poll the status of tun_busy ?

Or just return EBUSY if applications can decline interface destroying ?

[kevans]

(Sorry, I just realized I forgot about this review entirely)

In D39740#905088, @zlei wrote:

Shall we firstly signal the application that the tuntap device is going to be shutdown and destroyed and secondly poll the status of tun_busy ?

I'm not sure what that would look like, exactly.

Or just return EBUSY if applications can decline interface destroying ?

I think that's probably fine (in the may_intr case, since the !may_intr case is usually "we're evacuaating the vnet"), but I'd prefer to do as a follow-up so that we're not changing the semantics that much in a minor release (where-as, we can kinda fix the current situation a little bit)

[kevans]

Use a loop, set error before (it'll either get clobbered or remain untouched),
and just consistently keep it locked leaving the loop.

[kevans]

Ping

[markj]

sys/net/if_tuntap.c

632

I'd assert here that TUN_DYING isn't already set.

[kevans]

sys/net/if_tuntap.c

632

That gets a little more awkward with D44200, but I think it's solvable. For transient tunnels, we set TUN_DYING early so that we can safely drop the lock as we traverse through the if_clone infrastructure into tun_clone_destroy -> tun_destroy.

I think we just don't assert for transient tunnels, and transient tunnels override may_intr... if we have to wait, it's just long enough for the interface to be renamed right before we destroy it, because we already unbusied for the last close.

[markj]

sys/net/if_tuntap.c

632

The reason I suggested the assertion is to catch the case where this function is called twice somehow, as TUN_DYING isn't a terminal state anymore. I think omitting the assertion or checking (TUN_TRANSIENT | TUN_DYING) != TUN_DYING are both okay.

I don't quite follow what you mean about the interface being renamed?

[kevans]

sys/net/if_tuntap.c

632

We unbusied the transient tunnel on last close, but tunrename() might busy it to create a cdev alias if we got lucky enough to race interface rename against last close. We could actually have to wait here for make_dev_alias... to finish, otherwise we wouldn't have to consider where transient tunnel destruction should be interruptible

[markj]

sys/net/if_tuntap.c

632

We unbusy the transient tunnel on last close, but we also mark it as dying, preventing new busy references. What exactly is the race?

[kevans]

sys/net/if_tuntap.c

632

I was picturing another process racing SIOCSIFNAME (-> invokes tunrename() via an eventhandler) against last-close, since the former only requires a socket to perform -- in practice, I don't think creation of the /dev alias can block and we'll promptly exit cv_wait_sig() if these are happening concurrently.