Re: [PATCH net] net: phy: allow phy_detach() before netdev registration

[Chenguang Zhao <chenguang.zhao@linux.dev>]

On Sun, Sep 20, 2026 at 02:32:58AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] Incomplete fix: phy_detach() still emits the identical
>   "suspicious rcu_dereference_protected() usage" splat for net_devices…
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but the `struct
>   hwtstamp_provider` object stored in `dev->hwprov` has no free on the…
> 
> --
> 
> net: phy: allow phy_detach() before netdev registration
> 
> This changes phy_detach() to use rcu_dereference_protected() on dev->hwprov
> with a condition that accepts either RTNL being held or the net_device still
> being in NETREG_UNINITIALIZED state.  The goal is to silence a syzbot
> lockdep report from a probe-time phylink_connect_phy() failure path.
> 
> Closes: https://syzkaller.appspot.com/bug?extid=694b49f41098a5df4fd7
> 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 94b2e85e00a37..acd8d658a29e1 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -1941,7 +1941,9 @@ void phy_detach(struct phy_device *phydev)
> >  		/* hwprov may technically be protected by ops lock but
> >  		 * not for devices with a phydev, see phy_link_topo_add_phy()
> >  		 */
> > -		hwprov = rtnl_dereference(dev->hwprov);
> > +		hwprov = rcu_dereference_protected(dev->hwprov,
> > +						   lockdep_rtnl_is_held() ||
> > +						   dev->reg_state == NETREG_UNINITIALIZED);
> 
> [Severity: Medium]
> Does this cover all of the states in which the net_device is not visible to
> users?  The predicate only whitelists NETREG_UNINITIALIZED, so
> NETREG_UNREGISTERED still makes both operands false and the same
> rcu_dereference_protected() splat is printed.
> 
> One path that seems to still hit it is usbnet teardown, where
> unregister_netdev() runs before the driver unbind callback:
> 
> drivers/net/usb/usbnet.c:usbnet_disconnect() {
> 	...
> 	net = dev->net;
> 	unregister_netdev(net);
> 	...
> 	if (dev->driver_info->unbind)
> 		dev->driver_info->unbind(dev, intf);
> 	...
> }
> 
> By the time unbind() runs, netdev_run_todo() has already done:
> 
> net/core/dev.c:netdev_run_todo() {
> 	...
> 	WRITE_ONCE(dev->reg_state, NETREG_UNREGISTERED);
> 	...
> }
> 
> and smsc95xx_unbind() then reaches phy_detach() with no rtnl_lock() held:
> 
> drivers/net/usb/smsc95xx.c:smsc95xx_unbind() {
> 	...
> 	phy_disconnect(dev->net->phydev);
> 	...
> }
> 
> phy_disconnect() -> phy_detach(), reg_state is NETREG_UNREGISTERED, RTNL is
> not held, so is the warning unchanged for this caller?
> 
> A second path looks similar: when register_netdevice() fails after
> netdev_register_kobject(), it leaves
> 
> net/core/dev.c:register_netdevice() {
> 	...
> 	WRITE_ONCE(dev->reg_state, ret ? NETREG_UNREGISTERED : NETREG_REGISTERED);
> 	...
> }
> 
> and lan78xx_probe() then unwinds through its phy_uninit: label into
> lan78xx_phy_uninit() -> phylink_disconnect_phy() -> phy_detach() from probe
> context without RTNL.  That case also lands on NETREG_UNREGISTERED rather
> than NETREG_UNINITIALIZED.
> 
> Note that in the usbnet case dev->hwprov can be non-NULL, since it is
> installed while the device is registered by ethnl_set_tsconfig(), so
> execution continues into rcu_assign_pointer(dev->hwprov, NULL) and
> kfree_rcu() with neither RTNL nor the netdev ops lock annotated.
> 
> The kernel already expresses the "not user-visible" notion here:
> 
> include/net/netdev_lock.h:netdev_assert_locked_or_invisible() {
> 	if (dev->reg_state == NETREG_REGISTERED ||
> 	    dev->reg_state == NETREG_UNREGISTERING)
> 		netdev_assert_locked(dev);
> }
> 
> Would using that same notion, rather than a single state, close the whole
> warning class?
> 
> The commit message also says:
> 
>     hwprov cannot have been assigned before register_netdevice(). Relax the
>     RCU check for that state only; registered devices still require RTNL.
> 
> As written the code requires RTNL for unregistering and unregistered devices
> too, not just registered ones.  Should the wording be adjusted, or the
> condition widened to match?
> 
> >  		/* Disable timestamp if it is the one selected */
> >  		if (hwprov && hwprov->phydev == phydev) {
> >  			rcu_assign_pointer(dev->hwprov, NULL);
> 
> [Severity: Medium]
> This is a pre-existing issue and not introduced by this patch, but while
> looking at this block: is the struct hwtstamp_provider object ever freed on
> the net_device teardown path?
> 
> The only writers of dev->hwprov in the tree appear to be
> rcu_replace_pointer(dev->hwprov, hwprov, ...) in ethnl_set_tsconfig()
> (allocated by tsconfig_set_hwprov_from_desc() with hwprov = kzalloc_obj(*hwprov))
> and the rcu_assign_pointer(dev->hwprov, NULL) above.
> 
> net/core/dev.c has no reference to hwprov at all, so neither
> unregister_netdevice_many_notify() nor free_netdev() releases it.
> 
> Since the free in phy_detach() is conditional on hwprov->phydev == phydev, a
> provider with source HWTSTAMP_SOURCE_NETDEV (hwprov->phydev is NULL), or a
> HWTSTAMP_SOURCE_PHYLIB provider whose phydev is not the PHY being detached,
> never matches.  Does that leak the hwtstamp_provider allocation when the
> interface is destroyed after a tsconfig set?
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916021505.238990-1-chenguang.zhao%40linux.dev
This reply is based on v1. I have already sent v2. I can make further
changes to v1 if needed.

v2: https://lore.kernel.org/all/20260918022316.237646-1-chenguang.zhao@linux.dev/

pw-bot: cr