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:   Sat, 26 Mar 2022 13:39:29 +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 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.

Affected drivers:

          drivers/net/ethernet/actions/owl-emac.c
          drivers/net/ethernet/altera/altera_tse_main.c
          drivers/net/ethernet/apm/xgene-v2/mdio.c
          drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
          drivers/net/ethernet/arc/emac_main.c
          drivers/net/ethernet/asix/ax88796c_main.c
          drivers/net/ethernet/broadcom/tg3.c
          drivers/net/ethernet/cortina/gemini.c
          drivers/net/ethernet/dnet.c
          drivers/net/ethernet/ethoc.c
          drivers/net/ethernet/hisilicon/hip04_eth.c
          drivers/net/ethernet/lantiq_etop.c
          drivers/net/ethernet/marvell/pxa168_eth.c
          drivers/net/ethernet/toshiba/tc35815.c

Some of these use devm_register_netdev() on probe and disconnect the
PHY in their ->remove hook.  They're missing the fact that
__device_release_driver() calls the ->remove hook before
devres_release_all().

Thanks,

Lukas