Re: [PATCH net-next v2] net: phy: refuse to attach a PHY whose driver is being unbound

[Paolo Abeni]

On 9/19/26 03:53, Aleksei Sviridkin wrote:
> phy_remove() clears phydev->drv as its last act; the driver core clears
> d->driver only afterwards, in device_unbind_cleanup(). An attach entering
> that window still sees d->driver, so it skips the genphy substitution and
> then dereferences the NULL phydev->drv.
>
> Unbinding a PHY driver under an attached consumer crashes in real life.
> The board that showed it is an MT7981 whose copper PHY driver needs
> firmware from the rootfs, so the driver arrives after DSA has attached
> the PHY. I was testing how a port copes with that driver coming and
> going, on an OpenWrt 6.18 kernel with the distro's backports, local
> patches and the series under test. Racing a sysfs unbind against port
> teardown and bring-up oopsed twice: once in the state machine, and once
> inside a live phy_attach_direct() where phy_init_hw() had already
> entered the driver's config_init(). The second came after a
> one-line NULL check at the state-machine site let the run continue, with
> nothing added to widen the window. Both have this root. Neither is this
> dereference: in one the driver went away under a port close, in the other
> mid-attach, and in neither was phydev->drv already NULL when an attach
> started. That window is the narrow member of the family, and reaching it on
> demand needed a 200 ms msleep() at the end of phy_remove().
>
> Refuse the attach rather than let it complete on a driverless PHY, with
> -EBUSY, which this function already returns when the PHY is attached
> elsewhere. Without phylink the netdev would come up on a PHY that never ran
> config_init. Under phylink it does not get that far: phylink_bringup_phy()
> dereferences phy->drv->name as soon as the attach returns.
>
> The mid-attach case needs serialisation rather than a NULL test.
> phy_init_hw() tests phydev->drv once on entry and then dereferences it
> several more times, and it calls the driver's own config_init(), which is
> where one of those oopses landed, on a dereference made by driver code that
> no test in phylib can reach. The device lock is not available either:
> phy_attach_direct() runs under rtnl from ndo_open, while phy_remove() runs
> under the device lock and calls sfp_bus_del_upstream(), which takes rtnl
> for a PHY with an SFP bus.
>
> Assisted-by: LLM
> Signed-off-by: Aleksei Sviridkin <f@xxxxxx>
> ---
>
> Notes:
>      v1: https://lore.kernel.org/netdev/20260914204200.2743251-1-f@xxxxxx/
>      
>      v2:
>      - Said in the commit message where this was seen. The site in
>        phy_attach_direct() was found by reading the unbind path, but the race it
>        belongs to was not theoretical: it took the same board down twice with no
>        instrumentation in the kernel, while a sysfs unbind of the PHY driver was
>        raced against port teardown and bring-up. v1 opened with "found by
>        reading, not from a crash report", which was true of the site and
>        misleading about the race; that line is gone. The second of those two
>        came after the state-machine site had been given a local NULL test so the
>        run could continue, so the kernel that produced it carried that one extra
>        check; nothing was added to widen the window in either. The traces were
>        read at the time and the dumps were not preserved.

I read the above as you observed the addressed race without additional delays
in a real-life usage. That means target should be net.

More importantly if you observe and address a crash you should include the
relevant stack trace in the commit message.

Also I think that the sashiko feedback is relevant: you should attempt to close
the wider race, with proper locking. I read last part of the commit message as
a pre-existing lock inversion (RTNL should not be acquired under the device lock,
only vice-versa) prevents the more straight-forward fix. You should
provably investigate solving such lock inversion.

/P