if_tuntap: make SIOCIFDESTROY interruptible
Needs ReviewPublic
Actions

Authored by kevans on Apr 20 2023, 10:39 PM.

Details

Reviewers

None

Group Reviewers

network

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.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 56418
Build 53306: arc lint + arc unit

Event Timeline

kevans created this revision.Apr 20 2023, 10:39 PM

Herald added subscribers: glebius, melifaro, ae, imp. · View Herald TranscriptApr 20 2023, 10:39 PM

kevans requested review of this revision.Apr 20 2023, 10:39 PM

Harbormaster completed remote builds in B51085: Diff 120779.Apr 20 2023, 10:39 PM

kevans added a reviewer: network.Apr 20 2023, 10:40 PM

kevans mentioned this in D39722: if_tuntap: avoid a deadlock in SIOCIFDESTROY.

markj added a subscriber: markj.Apr 21 2023, 1:25 PM

markj added inline comments.

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.

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 ?

(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)