From owner-svn-src-head@freebsd.org Sat Oct 5 10:05:00 2019 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 0990D12E727 for ; Sat, 5 Oct 2019 10:05:00 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 46lj634sYMz4YYf; Sat, 5 Oct 2019 10:04:59 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id x95A4pXO031836 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Sat, 5 Oct 2019 13:04:54 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua x95A4pXO031836 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id x95A4oDd031835; Sat, 5 Oct 2019 13:04:50 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Sat, 5 Oct 2019 13:04:50 +0300 From: Konstantin Belousov To: John Baldwin Cc: Kyle Evans , src-committers , svn-src-head Subject: Re: svn commit: r353103 - head/sys/net Message-ID: <20191005100450.GU44691@kib.kiev.ua> References: <201910041343.x94Dh7Zo078270@repo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.12.2 (2019-09-21) X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FORGED_GMAIL_RCVD,FREEMAIL_FROM, NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on tom.home X-Rspamd-Queue-Id: 46lj634sYMz4YYf X-Spamd-Bar: ----- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-6.00 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-0.998,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 05 Oct 2019 10:05:00 -0000 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 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