Date: Sat, 5 Oct 2019 13:04:50 +0300 From: Konstantin Belousov <kostikbel@gmail.com> 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: <20191005100450.GU44691@kib.kiev.ua> 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 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@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20191005100450.GU44691>