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]

Date:   Tue, 15 Mar 2022 09:32:34 +0100
From:   Lukas Wunner <lukas@...ner.de>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     Andrew Lunn <andrew@...n.ch>, 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 Tue, Mar 15, 2022 at 06:44:03AM +0100, Oleksij Rempel wrote:
> 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:
> > > 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.
> > > > > 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.

Right, I was mistaken, sorry.


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

Unregistered yes, but freed no.  Here's the order before 2c9d6c2b871d:

  usbnet_disconnect()
    unregister_netdev()
    ax88772_unbind()
      phy_disconnect()
    free_netdev()

Is it illegal to disconnect a PHY from an unregistered, but not yet freed
net_device?

Oleksij, the commit message of 2c9d6c2b871d says that disconnecting the
PHY "fails" in that situation.  Please elaborate what the failure looked
like.  Did you get a stacktrace?

Thanks,

Lukas