Re: [PATCH net] net: usb: lan78xx: only stop phylink if the interface is running

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: usb: lan78xx: premature phylink teardown in lan78xx_disconnect()
--

commit eb1582b80d2c934b47efc3b66502d9e9f347ba7e
Author: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

net: usb: lan78xx: only stop phylink if the interface is running

This commit updates lan78xx_disconnect() to only call phylink_stop() when
netif_running() is true, avoiding a warning in phy_stop() when a USB device is
unplugged while the network interface is down.

> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 181e8c087655c..6dc622a6ae783 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -4594,7 +4594,8 @@ static void lan78xx_disconnect(struct usb_interface *intf)
>  	net = dev->net;
>  
>  	rtnl_lock();
> -	phylink_stop(dev->phylink);
> +	if (netif_running(net))
> +		phylink_stop(dev->phylink);
>  	phylink_disconnect_phy(dev->phylink);
>  	rtnl_unlock();

[Severity: Medium]
Does calling phylink_stop() and phylink_disconnect_phy() before
unregister_netdev(net) in drivers/net/usb/lan78xx.c:lan78xx_disconnect() cause
out-of-order teardown, a duplicate phylink_stop() call, and a race window when
rtnl_lock() is dropped?

In drivers/net/usb/lan78xx.c:lan78xx_disconnect():

	rtnl_lock();
	if (netif_running(net))
		phylink_stop(dev->phylink);
	phylink_disconnect_phy(dev->phylink);
	rtnl_unlock();

	unregister_netdev(net);

When netif_running(net) is true, calling phylink_stop(dev->phylink) here runs
lan78xx_mac_link_down() without holding dev->dev_mutex and while dev->napi,
TX/RX URBs, and dev->urb_intr are still active.

Because phylink_stop() does not clear __LINK_STATE_START, the subsequent
unregister_netdev(net) call sees that the interface is still running and
invokes ndo_stop -> lan78xx_stop():

drivers/net/usb/lan78xx.c:lan78xx_stop() {
	...
	mutex_lock(&dev->dev_mutex);
	...
	clear_bit(EVENT_DEV_OPEN, &dev->flags);
	napi_disable(&dev->napi);

	lan78xx_terminate_urbs(dev);
	...
	phylink_stop(dev->phylink);

	usb_kill_urb(dev->urb_intr);
	...
}

This executes phylink_stop(dev->phylink) a second time after
phylink_disconnect_phy(dev->phylink) has already detached the PHY.

In addition, releasing rtnl_unlock() before unregister_netdev(net) leaves net
registered while dev->phylink has already disconnected its PHY and freed the
PHY IRQ. In that window, a concurrent dev_open() -> lan78xx_open() can call
phylink_start(dev->phylink) on the disconnected phylink instance:

drivers/net/usb/lan78xx.c:lan78xx_open() {
	...
	phylink_start(dev->phylink);
	...
}

Similarly, dev->urb_intr remains active until lan78xx_stop() or later in
lan78xx_disconnect(), so an interrupt URB completion can still invoke
generic_handle_irq_safe(dev->domain_data.phyirq) after phy_disconnect() has
freed the IRQ.

Should lan78xx_disconnect() instead call unregister_netdev(net) first (which
automatically invokes lan78xx_stop() -> phylink_stop(dev->phylink) under
rtnl_lock() and dev->dev_mutex when the interface is running), and then call
phylink_disconnect_phy(dev->phylink) under rtnl_lock() afterwards?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029160204.2567-1-25031212351@stu.xidian.edu.cn?part=1