Re: [PATCH net-next v13 03/11] net: phy: add (*notify_phy_attach/detach)() hooks to struct mii_bus

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, leading to kernel panics and leaks.
--

--- Patch [3]: [PATCH net-next v13 03/11] net: phy: add (*notify_phy_attach/detach)() hooks to struct mii_bus ---
commit 379fa8ff9ef6381f09ee11a0d7f1d071da53e6f0
Author: Daniel Golle <[email protected]>

net: phy: add (*notify_phy_attach/detach)() hooks to struct mii_bus

This commit adds notify_phy_attach and notify_phy_detach hooks to the MDIO
bus to inform the driver when a PHY is being attached or detached. This avoids
reinventing bus scanning to determine PHYs and anticipates the right point in
time for operations like programming PHY polling registers.

Link: https://github.com/openwrt/openwrt/pull/21515#discussion_r2714069716
Link: https://lore.kernel.org/netdev/[email protected]/
Link: https://lore.kernel.org/netdev/[email protected]/#t

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a37..1a43fec022aa8 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_internal(phydev, false);
>  	return err;

[Severity: High]
This is a pre-existing issue, but does calling phy_detach_internal() here
destructively tear down the existing, valid attachment if the PHY is already
attached?

If a second caller attempts to attach an already-attached PHY, the check
earlier in phy_attach_direct() jumps to this error label:

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

The error path then calls phy_detach_internal(phydev, false). Looking at
phy_detach_internal(), it clears the original owner's state unconditionally:

    if (dev) {
            ...
            phydev->attached_dev->phydev = NULL;
            phydev->attached_dev = NULL;
            phy_link_topo_del_phy(dev, phydev);
    }

If the first MAC later dereferences its PHY pointer, could this lead to a
kernel panic? Because the first MAC's attachment state is broken, its own
teardown might also be skipped, potentially leaking device and module
references.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3