From owner-svn-src-head@freebsd.org Sat Oct 5 04:52:17 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 B35E0FECAE for ; Sat, 5 Oct 2019 04:52:17 +0000 (UTC) (envelope-from kevans@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 46lZ9F49d9z4JLq; Sat, 5 Oct 2019 04:52:17 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from mail-qt1-f172.google.com (mail-qt1-f172.google.com [209.85.160.172]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) (Authenticated sender: kevans) by smtp.freebsd.org (Postfix) with ESMTPSA id 687046E8A; Sat, 5 Oct 2019 04:52:17 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: by mail-qt1-f172.google.com with SMTP id 3so11658639qta.1; Fri, 04 Oct 2019 21:52:17 -0700 (PDT) X-Gm-Message-State: APjAAAXTEtra1l8DloQX2e1cHWWRaasuTmgXnnnOsVBxlXc35dTBgBIw dE6j5ypMFkdNRsYTKe/ptjNQrkxcuZpkXdiekZY= X-Google-Smtp-Source: APXvYqwxnFiKFnjzMrUow8cMjWDSQrP9LBzldT81APcb7g5fOhnmDy73YXT/2NmysX9xS/BQUTTUcM5Ffa8PvjGU2fY= X-Received: by 2002:a0c:ea92:: with SMTP id d18mr17457433qvp.112.1570251136594; Fri, 04 Oct 2019 21:52:16 -0700 (PDT) MIME-Version: 1.0 References: <201910041343.x94Dh7Zo078270@repo.freebsd.org> In-Reply-To: From: Kyle Evans Date: Fri, 4 Oct 2019 23:52:05 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r353103 - head/sys/net To: John Baldwin Cc: Kyle Evans , src-committers , svn-src-head Content-Type: text/plain; charset="UTF-8" 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 04:52:17 -0000 On Fri, Oct 4, 2019 at 5:24 PM 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. > 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