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

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

[Chenguang Zhao]

From: Chenguang Zhao <zhaochenguang@kylinos.cn>

phylink_connect_phy() may be called from probe without RTNL while the
net_device is still NETREG_UNINITIALIZED. If PHY bring-up fails,
phy_detach() uses rtnl_dereference(dev->hwprov) and lockdep reports
suspicious RCU usage.

hwprov cannot have been assigned before register_netdevice(). Relax the
RCU check for that state only; registered devices still require RTNL.

Fixes: 35f7cad1743e ("net: Add the possibility to support a selected hwtstamp in netdevice")
Reported-by: syzbot+694b49f41098a5df4fd7@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=694b49f41098a5df4fd7

Signed-off-by: Chenguang Zhao <zhaochenguang@kylinos.cn>
---
 drivers/net/phy/phy_device.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 94b2e85e00a3..acd8d658a29e 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);
 		/* Disable timestamp if it is the one selected */
 		if (hwprov && hwprov->phydev == phydev) {
 			rcu_assign_pointer(dev->hwprov, NULL);
-- 
2.25.1

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

[Andrew Lunn]

On Wed, Sep 16, 2026 at 10:15:05AM +0800, Chenguang Zhao wrote:
> From: Chenguang Zhao <zhaochenguang@kylinos.cn>
> 
> phylink_connect_phy() may be called from probe without RTNL while the
> net_device is still NETREG_UNINITIALIZED. If PHY bring-up fails,
> phy_detach() uses rtnl_dereference(dev->hwprov) and lockdep reports
> suspicious RCU usage.

What do you mean by PHY bring-up?

     Andrew

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

[Chenguang Zhao]

On Wed, Sep 16, 2026 at 02:12:27PM +0200, Andrew Lunn wrote:
> On Wed, Sep 16, 2026 at 10:15:05AM +0800, Chenguang Zhao wrote:
> > From: Chenguang Zhao <zhaochenguang@kylinos.cn>
> > 
> > phylink_connect_phy() may be called from probe without RTNL while the
> > net_device is still NETREG_UNINITIALIZED. If PHY bring-up fails,
> > phy_detach() uses rtnl_dereference(dev->hwprov) and lockdep reports
> > suspicious RCU usage.
> 
> What do you mean by PHY bring-up?
> 
>      Andrew
Hi Andrew

PHY bring-up refers to phylink_bringup_phy(). The call chain is as follows:

usbnet_probe
  -> ax88772_bind
       -> ax88772_init_phy
            -> phylink_connect_phy
                 -> phylink_attach_phy / phy_attach_direct   // suceess 
                 -> phylink_bringup_phy                      // fail 
                 -> phy_detach                               // No RTNL
                      -> rtnl_dereference(dev->hwprov)       // lockdep warning

Because RTNL is not held when phy_detach() is called, rtnl_dereference() triggers a lockdep warning.

Thanks
Chenguang

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

[Andrew Lunn]

On Thu, Sep 17, 2026 at 03:06:46PM +0800, Chenguang Zhao wrote:
> On Wed, Sep 16, 2026 at 02:12:27PM +0200, Andrew Lunn wrote:
> > On Wed, Sep 16, 2026 at 10:15:05AM +0800, Chenguang Zhao wrote:
> > > From: Chenguang Zhao <zhaochenguang@kylinos.cn>
> > > 
> > > phylink_connect_phy() may be called from probe without RTNL while the
> > > net_device is still NETREG_UNINITIALIZED. If PHY bring-up fails,
> > > phy_detach() uses rtnl_dereference(dev->hwprov) and lockdep reports
> > > suspicious RCU usage.
> > 
> > What do you mean by PHY bring-up?
> > 
> >      Andrew
> Hi Andrew
> 
> PHY bring-up refers to phylink_bringup_phy(). The call chain is as follows:
> 
> usbnet_probe
>   -> ax88772_bind
>        -> ax88772_init_phy
>             -> phylink_connect_phy
>                  -> phylink_attach_phy / phy_attach_direct   // suceess 
>                  -> phylink_bringup_phy                      // fail 
>                  -> phy_detach                               // No RTNL
>                       -> rtnl_dereference(dev->hwprov)       // lockdep warning
> 
> Because RTNL is not held when phy_detach() is called, rtnl_dereference() triggers a lockdep warning.

Lets zoom out and look at the big picture....

Why is phy_detach() not symmetric with phy_attach()?

Why is phy_detach() touching dev->hwprov, when phy_attach() does not?

Where is dev->hwprov set and why is the mirror function not dealing
with this?

     Andrew

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

[netdev-bot+sashiko]

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

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

[Chenguang Zhao]

On Thu, Sep 17, 2026 at 02:04:51PM +0200, Andrew Lunn wrote:
> On Thu, Sep 17, 2026 at 03:06:46PM +0800, Chenguang Zhao wrote:
> > On Wed, Sep 16, 2026 at 02:12:27PM +0200, Andrew Lunn wrote:
> > > On Wed, Sep 16, 2026 at 10:15:05AM +0800, Chenguang Zhao wrote:
> > > > From: Chenguang Zhao <zhaochenguang@kylinos.cn>
> > > > 
> > > > phylink_connect_phy() may be called from probe without RTNL while the
> > > > net_device is still NETREG_UNINITIALIZED. If PHY bring-up fails,
> > > > phy_detach() uses rtnl_dereference(dev->hwprov) and lockdep reports
> > > > suspicious RCU usage.
> > > 
> > > What do you mean by PHY bring-up?
> > > 
> > >      Andrew
> > Hi Andrew
> > 
> > PHY bring-up refers to phylink_bringup_phy(). The call chain is as follows:
> > 
> > usbnet_probe
> >   -> ax88772_bind
> >        -> ax88772_init_phy
> >             -> phylink_connect_phy
> >                  -> phylink_attach_phy / phy_attach_direct   // suceess 
> >                  -> phylink_bringup_phy                      // fail 
> >                  -> phy_detach                               // No RTNL
> >                       -> rtnl_dereference(dev->hwprov)       // lockdep warning
> > 
> > Because RTNL is not held when phy_detach() is called, rtnl_dereference() triggers a lockdep warning.
> 
> Lets zoom out and look at the big picture....
> 
> Why is phy_detach() not symmetric with phy_attach()?
> 
> Why is phy_detach() touching dev->hwprov, when phy_attach() does not?
> 
> Where is dev->hwprov set and why is the mirror function not dealing
> with this?
The dev->hwprov is set by ETHTOOL_MSG_TSCONFIG_SET in
ethnl_set_tsconfig(), which is the only writer besides phy_detach().

phy_detach() clears hwprov only when the selected provider is this PHY,
so skb_clone_tx_timestamp() / skb_defer_rx_timestamp() do not keep a
dangling phydev pointer after the PHY is gone. That is not the reverse
of phy_attach(); it is lifetime cleanup for a pointer that ethtool
stashed in the netdev.

Chenguang
Thanks
> 
>      Andrew

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

[Chenguang Zhao]

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

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

[Andrew Lunn]

> > Lets zoom out and look at the big picture....
> > 
> > Why is phy_detach() not symmetric with phy_attach()?
> > 
> > Why is phy_detach() touching dev->hwprov, when phy_attach() does not?
> > 
> > Where is dev->hwprov set and why is the mirror function not dealing
> > with this?

> The dev->hwprov is set by ETHTOOL_MSG_TSCONFIG_SET in
> ethnl_set_tsconfig(), which is the only writer besides phy_detach().

So this is the real problem. It does not fit the usual pattern of
setup and tairdown.

> phy_detach() clears hwprov only when the selected provider is this PHY,
> so skb_clone_tx_timestamp() / skb_defer_rx_timestamp() do not keep a
> dangling phydev pointer after the PHY is gone. That is not the reverse
> of phy_attach(); it is lifetime cleanup for a pointer that ethtool
> stashed in the netdev.

Can we make it fit the usual pattern?

https://elixir.bootlin.com/linux/v7.2.5/source/net/ethtool/tsconfig.c#L413

This sets the stamper to zero_config, via dev_set_hwtstamp_phylib().
Could that be done in phy_attach()?  phy_detach() would then have the
opposite, call dev_clear_hwtstamp_phylib().

ethnl_set_tsconfig() then just replaces the setting?

I would put dev_clear_hwtstamp_phylib() next to
dev_set_hwtstamp_phylib() so it is obvious the locking is the same in
both.

	Andrew

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

[Chenguang Zhao]

On Mon, Sep 21, 2026 at 02:15:58PM +0200, Andrew Lunn wrote:
> > > Lets zoom out and look at the big picture....
> > > 
> > > Why is phy_detach() not symmetric with phy_attach()?
> > > 
> > > Why is phy_detach() touching dev->hwprov, when phy_attach() does not?
> > > 
> > > Where is dev->hwprov set and why is the mirror function not dealing
> > > with this?
> 
> > The dev->hwprov is set by ETHTOOL_MSG_TSCONFIG_SET in
> > ethnl_set_tsconfig(), which is the only writer besides phy_detach().
> 
> So this is the real problem. It does not fit the usual pattern of
> setup and tairdown.
> 
> > phy_detach() clears hwprov only when the selected provider is this PHY,
> > so skb_clone_tx_timestamp() / skb_defer_rx_timestamp() do not keep a
> > dangling phydev pointer after the PHY is gone. That is not the reverse
> > of phy_attach(); it is lifetime cleanup for a pointer that ethtool
> > stashed in the netdev.
> 
> Can we make it fit the usual pattern?
> 
> https://elixir.bootlin.com/linux/v7.2.5/source/net/ethtool/tsconfig.c#L413
> 
> This sets the stamper to zero_config, via dev_set_hwtstamp_phylib().
> Could that be done in phy_attach()?  phy_detach() would then have the
> opposite, call dev_clear_hwtstamp_phylib().
> 
> ethnl_set_tsconfig() then just replaces the setting?
> 
> I would put dev_clear_hwtstamp_phylib() next to
> dev_set_hwtstamp_phylib() so it is obvious the locking is the same in
> both.
> 
> 	Andrew

Hi Andrew,

I've just sent out v3, addressing your comments on the provider lifetime
asymmetry.

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

Happy to iterate further if anything still needs adjustment.

Thanks,
Chenguang