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

[netdev-bot+sashiko@kernel.org]

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