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: <20220315054403.GA14588@pengutronix.de>
Date:   Tue, 15 Mar 2022 06:44:03 +0100
From:   Oleksij Rempel <o.rempel@...gutronix.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Lukas Wunner <lukas@...ner.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 Mon, Mar 14, 2022 at 08:14:36PM +0100, Andrew Lunn wrote:
> On Mon, Mar 14, 2022 at 07:42:34PM +0100, Lukas Wunner wrote:
> > [cc += Heiner Kallweit, Andrew Lunn]
> > 
> > On Thu, Mar 10, 2022 at 12:38:20PM +0100, Oleksij Rempel wrote:
> > > On Thu, Mar 10, 2022 at 12:25:08PM +0100, Oliver Neukum wrote:
> > > > I got bug reports that 2c9d6c2b871d ("usbnet: run unbind() before
> > > > unregister_netdev()")
> > > > is causing regressions.
> > 
> > I would like to see this reverted as well.  For obvious reasons,
> > the order in usbnet_disconnect() should be the inverse of
> > usbnet_probe().  Since 2c9d6c2b871d, that's no longer the case.
> > 
> > 
> > > > Rather than simply reverting it,
> > > > it seems to me that the call needs to be split. One in the old place
> > > > and one in the place you moved it to.
> > 
> > I disagree.  The commit message claims that the change is necessary
> > because phy_disconnect() fails if called with phydev->attached_dev == NULL.
> 
> The only place i see which sets phydev->attached_dev is
> phy_attach_direct(). So if phydev->attached_dev is NULL, the PHY has
> not been attached, and hence there is no need to call
> phy_disconnect().

phydev->attached_dev is not NULL. It was linked to unregistered/freed
netdev. This is why my patch changing the order to call phy_disconnect()
first and then unregister_netdev().

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |