svn commit: r353103 - head/sys/net

[Konstantin Belousov]

On Fri, Oct 04, 2019 at 03:24:45PM -0700, John Baldwin wrote:
> 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.
Unfortunately not.  The same complications are created by multi-mounts
of devfs and forced unmount.

You never know that d_close() is really last, and D_TRACKCLOSE is unable
to provide the guarantee that each open is matched.

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