Re: [PATCH net-next v12 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 and leaks refcounts.
--

--- Patch [3]: [PATCH net-next v12 03/11] net: phy: add (*notify_phy_attach/detach)() hooks to struct mii_bus ---
commit f476eabef52740ac2e24b92c70182265b4497974
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 driver. This allows MDIO bus drivers to handle PHY polling registers
appropriately without reinventing bus scanning. It also refactors phy_detach()
into phy_detach_internal() to support symmetric notification.

[1] https://github.com/openwrt/openwrt/pull/21515#discussion_r2714069716
[2] https://lore.kernel.org/netdev/[email protected]/
[3] 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
> @@ -1734,6 +1734,96 @@ static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
>  	return phydrv->config_intr && phydrv->handle_interrupt;
>  }
>  
> +static void phy_detach_internal(struct phy_device *phydev, bool notify_bus)
> +{
> +	struct net_device *dev = phydev->attached_dev;

[ ... ]

> +	if (dev) {
> +		struct hwtstamp_provider *hwprov;
> +
> +		/* 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);
> +		/* Disable timestamp if it is the one selected */
> +		if (hwprov && hwprov->phydev == phydev) {
> +			rcu_assign_pointer(dev->hwprov, NULL);
> +			kfree_rcu(hwprov, rcu_head);
> +		}
> +
> +		phydev->attached_dev->phydev = NULL;
> +		phydev->attached_dev = NULL;
> +		phy_link_topo_del_phy(dev, phydev);
> +	}

[ ... ]

> @@ -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 original attachment?

If phy_attach_direct() is called on a PHY that is already attached, it detects
this with:

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

Because this jumps to the error label and invokes phy_detach_internal() with the
existing phydev->attached_dev, does it tear down the state of the original
network device attachment while leaving its initial refcounts leaked?

This could leave the original device with a nullified PHY pointer, potentially
crashing when accessed.

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