From owner-svn-src-head@freebsd.org Fri Oct 4 22:24:48 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 379C713B49D for ; Fri, 4 Oct 2019 22:24:48 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 46lPZ801bSz3xmr; Fri, 4 Oct 2019 22:24:48 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from John-Baldwins-MacBook-Pro-4.local (ralph.baldwin.cx [66.234.199.215]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 63E5D4233; Fri, 4 Oct 2019 22:24:47 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Subject: Re: svn commit: r353103 - head/sys/net To: Kyle Evans Cc: src-committers , svn-src-head References: <201910041343.x94Dh7Zo078270@repo.freebsd.org> From: John Baldwin Openpgp: preference=signencrypt Autocrypt: addr=jhb@FreeBSD.org; keydata= mQGiBETQ+XcRBADMFybiq69u+fJRy/0wzqTNS8jFfWaBTs5/OfcV7wWezVmf9sgwn8TW0Dk0 c9MBl0pz+H01dA2ZSGZ5fXlmFIsee1WEzqeJzpiwd/pejPgSzXB9ijbLHZ2/E0jhGBcVy5Yo /Tw5+U/+laeYKu2xb0XPvM0zMNls1ah5OnP9a6Ql6wCgupaoMySb7DXm2LHD1Z9jTsHcAQMD /1jzh2BoHriy/Q2s4KzzjVp/mQO5DSm2z14BvbQRcXU48oAosHA1u3Wrov6LfPY+0U1tG47X 1BGfnQH+rNAaH0livoSBQ0IPI/8WfIW7ub4qV6HYwWKVqkDkqwcpmGNDbz3gfaDht6nsie5Z pcuCcul4M9CW7Md6zzyvktjnbz61BADGDCopfZC4of0Z3Ka0u8Wik6UJOuqShBt1WcFS8ya1 oB4rc4tXfSHyMF63aPUBMxHR5DXeH+EO2edoSwViDMqWk1jTnYza51rbGY+pebLQOVOxAY7k do5Ordl3wklBPMVEPWoZ61SdbcjhHVwaC5zfiskcxj5wwXd2E9qYlBqRg7QeSm9obiBCYWxk d2luIDxqaGJARnJlZUJTRC5vcmc+iGAEExECACAFAkTQ+awCGwMGCwkIBwMCBBUCCAMEFgID AQIeAQIXgAAKCRBy3lIGd+N/BI6RAJ9S97fvbME+3hxzE3JUyUZ6vTewDACdE1stFuSfqMvM jomvZdYxIYyTUpC5Ag0ERND5ghAIAPwsO0B7BL+bz8sLlLoQktGxXwXQfS5cInvL17Dsgnr3 1AKa94j9EnXQyPEj7u0d+LmEe6CGEGDh1OcGFTMVrof2ZzkSy4+FkZwMKJpTiqeaShMh+Goj XlwIMDxyADYvBIg3eN5YdFKaPQpfgSqhT+7El7w+wSZZD8pPQuLAnie5iz9C8iKy4/cMSOrH YUK/tO+Nhw8Jjlw94Ik0T80iEhI2t+XBVjwdfjbq3HrJ0ehqdBwukyeJRYKmbn298KOFQVHO EVbHA4rF/37jzaMadK43FgJ0SAhPPF5l4l89z5oPu0b/+5e2inA3b8J3iGZxywjM+Csq1tqz hltEc7Q+E08AAwUIAL+15XH8bPbjNJdVyg2CMl10JNW2wWg2Q6qdljeaRqeR6zFus7EZTwtX sNzs5bP8y51PSUDJbeiy2RNCNKWFMndM22TZnk3GNG45nQd4OwYK0RZVrikalmJY5Q6m7Z16 4yrZgIXFdKj2t8F+x613/SJW1lIr9/bDp4U9tw0V1g3l2dFtD3p3ZrQ3hpoDtoK70ioIAjjH aIXIAcm3FGZFXy503DOA0KaTWwvOVdYCFLm3zWuSOmrX/GsEc7ovasOWwjPn878qVjbUKWwx Q4QkF4OhUV9zPtf9tDSAZ3x7QSwoKbCoRCZ/xbyTUPyQ1VvNy/mYrBcYlzHodsaqUDjHuW+I SQQYEQIACQUCRND5ggIbDAAKCRBy3lIGd+N/BCO8AJ9j1dWVQWxw/YdTbEyrRKOY8YZNwwCf afMAg8QvmOWnHx3wl8WslCaXaE8= Message-ID: Date: Fri, 4 Oct 2019 15:24:45 -0700 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Fri, 04 Oct 2019 22:24:48 -0000 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. > 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