Re: [PATCH net-next v7 3/8] net: phy: add (*notify_phy_attach/detach)() hooks to struct mii_bus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling `phy_attach_direct()` on an already-attached PHY destructively tears down the existing attachment and corrupts module refcounts.
--

--- Patch [3]: [PATCH net-next v7 3/8] net: phy: add (*notify_phy_attach/detach)() hooks to struct mii_bus ---
commit f981bcd4f5183a24448425da1f5f3cc2787f456b
Author: Daniel Golle <daniel@makrotopia.org>
Subject: net: phy: add (*notify_phy_attach/detach)() hooks to struct mii_bus

This commit adds notify_phy_attach() and notify_phy_detach() hooks to
struct mii_bus. This allows MDIO bus drivers to be informed when a PHY
is attached or detached. The teardown logic from phy_detach() is moved
into a new __phy_detach() helper to handle the notification conditionally.

Link: https://github.com/openwrt/openwrt/pull/21515#discussion_r2714069716
Link: https://lore.kernel.org/netdev/cover.1769053496.git.daniel@makrotopia.org/

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a37..233622e9ba61f 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1890,8 +1986,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	return err;
>  
>  error:
> -	/* phy_detach() does all of the cleanup below */
> -	phy_detach(phydev);
> +	/* cleanup without bus notification */
> +	__phy_detach(phydev, false);

[Severity: High]
This is a pre-existing issue, but is it safe to call __phy_detach() here
if we reached this error path because the PHY was already attached?

Earlier in phy_attach_direct(), if the PHY is already attached, it jumps
to the error path:

	if (phydev->attached_dev) {
		dev_err(&dev->dev, "PHY already attached\n");
		err = -EBUSY;
		goto error;
	}

By calling __phy_detach(), it destructively tears down the existing attachment.
It nullifies phydev->attached_dev and removes links:

drivers/net/phy/phy_device.c:__phy_detach() {
	...
	phydev->attached_dev->phydev = NULL;
	phydev->attached_dev = NULL;
	phy_link_topo_del_phy(dev, phydev);
	...
}

Could this lead to unexpected device detachment and potential use-after-free
or null-pointer dereferences in the original owner's driver?

Does this also corrupt the module refcount for bus->owner? The module_get()
in phy_attach_direct() is conditional on the new device's owner, while
the module_put() in __phy_detach() relies on the old device's owner.

Would it be better to add a separate error label that skips __phy_detach()
for cases where the attachment hasn't been established yet?

>  	return err;
>  
>  error_module_put:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260726071751.1359156-1-markus.stockhausen@gmx.de?part=3