Re: [PATCH net-next v10 03/10] 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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] When `phy_attach_direct()` is called on a PHY device that is already attached, it returns `-EBUSY` but erroneously jumps to a destructive error path that tears down the existing valid attachment.
- [High] Unsafe NULL pointer dereferences occur in `phy_attach_direct()` when resolving module owners and logging errors for a standalone PHY (where `dev` is NULL) or when a network device lacks a parent hierarchy.
--

--- Patch [3]: [PATCH net-next v10 03/10] net: phy: add (*notify_phy_attach/detach)() hooks to struct mii_bus ---
commit 22ccb291ed876048a65b9818306c88cf227879fa
Author: Daniel Golle <daniel@makrotopia.org>

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 to allow MDIO bus drivers to handle PHY attach/detach events.
It also extracts a __phy_detach helper to support these notifications.

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
> @@ -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(struct phy_device *phydev, bool notify_bus)
> +{
> +	struct net_device *dev = phydev->attached_dev;
> +	struct module *ndev_owner = NULL;
> +	struct mii_bus *bus;
> +

[ ... ]

> +	/*
> +	 * The phydev might go away on the put_device() below, so avoid
> +	 * a use-after-free bug by reading the underlying bus first.
> +	 */
> +	bus = phydev->mdio.bus;
> +
> +	put_device(&phydev->mdio.dev);
> +	if (dev)
> +		ndev_owner = dev->dev.parent->driver->owner;

[Severity: High]
This is a pre-existing issue, but could this code cause a NULL pointer
dereference for virtual network devices?

If dev->dev.parent is NULL (which can happen for virtual network devices)
or if dev->dev.parent->driver is NULL (such as for unbound parent devices),
dereferencing driver->owner will panic.

> +	if (ndev_owner != bus->owner)
> +		module_put(bus->owner);
> +}

[ ... ]

> @@ -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 does this error path inadvertently destroy
existing valid attachments?

In phy_attach_direct(), if the PHY is already attached:

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

The code jumps to the error path and unconditionally dismantles the
pre-existing attachment instead of aborting the duplicate request.

Additionally, if phy_attach_direct() is called with a NULL dev (e.g. for a
standalone PHY) and the PHY is already attached, the dev_err() call above
would dereference the NULL dev pointer.

>  	return err;

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