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: <Yjh5Qz8XX1ltiRUM@lunn.ch>
Date:   Mon, 21 Mar 2022 14:10:27 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Lukas Wunner <lukas@...ner.de>
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

> It turns out that unregistering a net_device and then calling
> phy_disconnect() may indeed crash and is thus not permitted
> right now:
> 
> Oleksij added the following code comment with commit e532a096be0e
> ("net: usb: asix: ax88772: add phylib support"):
> 
>   /* On unplugged USB, we will get MDIO communication errors and the
>    * PHY will be set in to PHY_HALTED state.
> 
> So the USB adapter gets unplugged, access to MII registers fails with
> -ENODEV, phy_error() is called, phy_state_machine() transitions to
> PHY_HALTED and performs the following call:
> 
>   phy_state_machine()
>     phy_link_down()
>       phy_link_change()
>         netif_carrier_off()
>           linkwatch_fire_event()
> 
> Asynchronously, usbnet_disconnect() calls phy_detach() and then
> free_netdev().
> 
> A bit later, linkwatch_event() runs and tries to access the freed
> net_device, leading to the crash that Oleksij posted upthread.
> 
> The fact that linkwatch_fire_event() increments the refcount doesn't
> help because unregister_netdevice() has already run (it waits for
> the refcount to drop to 1).
> 
> My suggestion would be to amend unregister_netdevice() to set
> dev->phydev->attached_dev = NULL.  It may also be a good idea
> to WARN_ON() in free_netdev() if the refcount is not 1.
> 
> Andrew, please clarify whether you really think that the
> "unregister netdev, then detach phy" order should be supported.

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.

A WARN_ON() for refcount not 1 makes a lot of sense.

dev->phydev->attached_dev = NULL im less sure about. That is what
detach is all about. I think we need to review the code and see if the
normal path is making use of netdev, or is this problem limited to
phy_error(), which the USB code is invoking? It maybe we need to make
phy_error() more paranoid, or not even used in detach.

	    Andrew