netdev - Re: ordering of call to unbind() in usbnet

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220326130430.GD31022@wunner.de>
Date:   Sat, 26 Mar 2022 14:04:30 +0100
From:   Lukas Wunner <lukas@...ner.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Oleksij Rempel <o.rempel@...gutronix.de>,
        Oliver Neukum <oneukum@...e.com>,
        Oleksij Rempel <linux@...pel-privat.de>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: ordering of call to unbind() in usbnet_disconnect

On Sat, Mar 26, 2022 at 01:49:28PM +0100, Andrew Lunn wrote:
> On Sat, Mar 26, 2022 at 01:39:29PM +0100, Lukas Wunner wrote:
> > On Mon, Mar 21, 2022 at 02:10:27PM +0100, Andrew Lunn wrote:
> > > There are two patterns in use at the moment:
> > > 
> > > 1) The phy is attached in open() and detached in close(). There is no
> > >    danger of the netdev disappearing at this time.
> > > 
> > > 2) The PHY is attached during probe, and detached during release.
> > > 
> > > This second case is what is being used here in the USB code. This is
> > > also a common pattern for complex devices. In probe, you get all the
> > > components of a complex devices, stitch them together and then
> > > register the composite device. During release, you unregister the
> > > composite device, and then release all the components. Since this is a
> > > natural model, i think it should work.
> > 
> > I've gone through all drivers and noticed that some of them use a variation
> > of pattern 2 which looks fishy:
> > 
> > On probe, they first attach the PHY, then register the netdev.
> > On remove, they detach the PHY, then unregister the netdev.
> > 
> > Is it legal to detach the PHY from a registered (potentially running)
> > netdev? It looks wrong to me.
> 
> I think the network stack guarantee that the close() method is called
> before unregister completes. It is a common pattern to attach the PHY
> in open() and detach it in close(). The stack itself should not be
> using the PHY when it is down, the exception being IOCTL handlers
> which people often get wrong.

But the PHY is detached from a *running* netdev *before* that netdev
is unregistered (and closed).  Is that really legal?

Thanks,

Lukas