Date: Fri, 4 Oct 2019 23:52:05 -0500 From: Kyle Evans <kevans@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: Kyle Evans <kevans@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r353103 - head/sys/net Message-ID: <CACNAnaFnUcZ_8kcyjn1nSSffQam8cxkU3Zs1%2Brx3vP69DfKRJg@mail.gmail.com> In-Reply-To: <a32a3631-cf75-c82f-6208-d397a701d138@FreeBSD.org> References: <201910041343.x94Dh7Zo078270@repo.freebsd.org> <ece67d32-2624-4e06-08a6-5d67aa4a2e03@FreeBSD.org> <CACNAnaG-cbK6VtJA1S6_zL7M=QpTwBS6WytbJLjK71yZOsBgBA@mail.gmail.com> <a32a3631-cf75-c82f-6208-d397a701d138@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Oct 4, 2019 at 5:24 PM John Baldwin <jhb@freebsd.org> wrote: > > On 10/4/19 1:48 PM, Kyle Evans wrote: > > On Fri, Oct 4, 2019 at 2:12 PM John Baldwin <jhb@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. > Ahh- that makes sense. > 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. > Hmm, this seems to be a little wart-y in conjunction with cloned devices. AFAICT, you *have* to clone_create to attempt to locate a device on the clone list. If it can't be found, one gets prepped at that unit number, placed on the clone list, and subsequently returned later in newdev when you attempt to make_dev/make_dev_s to actually initialize the device. Because it now already exists, mda_si_drv1 won't get copied over to the device. I think newdev needs to be taught that SI_CLONELIST && !SI_NAMED is an uninitialized clone device that should also initialize the arguments. Something like this: https://people.freebsd.org/~kevans/cloned-makedev.diff -> though the si_devsw setting should be redundant in this case, it's probably better to do this goto and just exempt the one part that doesn't apply to cloned devices rather than duplicate the assignments. thanks, Kyle Evans
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACNAnaFnUcZ_8kcyjn1nSSffQam8cxkU3Zs1%2Brx3vP69DfKRJg>