svn commit: r353103 - head/sys/net

[John Baldwin]

On 10/4/19 1:48 PM, Kyle Evans wrote:
> On Fri, Oct 4, 2019 at 2:12 PM John Baldwin <jhb at freebsd.org> wrote:
>>
>> On 10/4/19 6:43 AM, Kyle Evans wrote:
>>> Author: kevans
>>> Date: Fri Oct  4 13:43:07 2019
>>> New Revision: 353103
>>> URL: https://svnweb.freebsd.org/changeset/base/353103
>>>
>>> Log:
>>>   tuntap(4): loosen up tunclose restrictions
>>>
>>>   Realistically, this cannot work. We don't allow the tun to be opened twice,
>>>   so it must be done via fd passing, fork, dup, some mechanism like these.
>>>   Applications demonstrably do not enforce strict ordering when they're
>>>   handing off tun devices, so the parent closing before the child will easily
>>>   leave the tun/tap device in a bad state where it can't be destroyed and a
>>>   confused user because they did nothing wrong.
>>>
>>>   Concede that we can't leave the tun/tap device in this kind of state because
>>>   of software not playing the TUNSIFPID game, but it is still good to find and
>>>   fix this kind of thing to keep ifconfig(8) up-to-date and help ensure good
>>>   discipline in tun handling.
>>
>> Why are you using d_close for last close anyway?  It's not really reliable compared
>> to using cdevpriv and a cdevpriv dtor.

Sorry, right after I sent this I realized that is probably just the old code.

> This decision predates me by a long time, I'm afraid. =-)
> 
> If you have time to elaborate on the comparable reliability point, I'd
> be interested in hearing it. A little bit of searching didn't seem to
> turn up much there, I'm afraid.

There are certain edge cases (e.g. if d_open() fails part-way through IIRC, but
I think at least one other) where d_close() may not get invoked.  OTOH, once the
cdevpriv dtor is installed, it will always be called when the reference count on
the associated struct file drops to zero.  If you need to reliably free
resources created during open(), then the cdevpriv dtor is the best way to
manage that.

d_close() can still be useful for dealing with conditions that aren't "this file
descriptor has completely gone away" since close() can have defined semantics on
ttys as Bruce has noted.  Most non-tty devices don't honor revoke(), and it's not
fully clear to me if revoke() really makes sense on non-tty devices.  (E.g. what
does it mean to revoke a swap partition, /dev/null, or /dev/random)  Without
revoke() I think you avoid many of the complexities from close semantics.

> I did otherwise spend a little bit of time diving into the path taken
> to get to d_close and the trade-offs between cdevpriv vs. what tuntap
> does now. I think I'm convinced either way that cdevpriv is a good way
> to go- it seems to have the advantage that with a little refactoring
> we could actually set the softc atomically on the device cdevpriv
> instead of cdev->si_drv1 and I can axe this rwatson@ comment about the
> non-atomic test and set.

So the si_drv1 thing doesn't require cdevpriv, that is just a matter of using
make_dev_s() instead of make_dev().  Any driver that uses si_drv1 should really
be using make_dev_s() to close the race of setting si_drv1 during cdev
creation.

-- 
John Baldwin