Re: [PATCH net-next v15 04/13] net: phy: add phy_detach_internal() helper

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] phy_attach_direct() error path destructively tears down pre-existing PHY attachments and corrupts module refcounts.
--

commit 9c0668542a6d1a738f40f7d8a8ebdeecb5d1257f
Author: Markus Stockhausen <markus.stockhausen@gmx.de>
Subject: net: phy: add phy_detach_internal() helper

This patch carves out the phy_detach() code into a new helper
phy_detach_internal() and makes phy_detach() a short stub that calls the
new helper. This prepares for upcoming MDIO bus notification enhancements
that need to distinguish between normal detach and cleanup paths.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 8cb0d60fcbba9..69b04a92b7766 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1763,6 +1763,98 @@ static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
>  	return phydrv->config_intr && phydrv->handle_interrupt;
>  }
>  
> +/**
> + * phy_detach_internal - detach a PHY device from its network device
> + * @phydev: target phy_device struct
> + *
> + * This detaches the phy device from its network device and the phy
> + * driver, and drops the reference count taken in phy_attach_direct().
> + */
> +static void phy_detach_internal(struct phy_device *phydev)
> +{
> +	struct net_device *dev = phydev->attached_dev;
> +	struct module *ndev_owner = NULL;
> +	struct mii_bus *bus;

[ ... ]

> +	phy_suspend(phydev);
> +	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);
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but this logic can tear down
an existing attachment when an error occurs during phy_attach_direct().

If phy_attach_direct() is called on a PHY that is already attached to
another device, it hits this check:

drivers/net/phy/phy_device.c:phy_attach_direct() {
	if (phydev->attached_dev) {
		dev_err(&dev->dev, "PHY already attached\n");
		err = -EBUSY;
		goto error;
	}
...
error:
	/* phy_detach() does all of the cleanup below */
	phy_detach(phydev);
	return err;
}

This calls phy_detach_internal() on the old attached_dev, completely
destroying the pre-existing attachment (clearing its phydev pointer,
suspending the PHY, etc.) without notifying it. Can we abort harmlessly
instead of destructively modifying the pre-existing state?

> +
> +	phydev->phy_link_change = NULL;
> +	phydev->phylink = NULL;
> +
> +	if (phydev->mdio.dev.driver)
> +		module_put(phydev->mdio.dev.driver->owner);
> +
> +	/* If the device had no specific driver before (i.e. - it
> +	 * was using the generic driver), we unbind the device
> +	 * from the generic driver so that there's a chance a
> +	 * real driver could be loaded
> +	 */
> +	if (phydev->is_genphy_driven) {
> +		device_release_driver(&phydev->mdio.dev);
> +		phydev->is_genphy_driven = 0;
> +	}
> +
> +	/* Assert the reset signal */
> +	phy_device_reset(phydev, 1);
> +
> +	/*
> +	 * 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;
> +	if (ndev_owner != bus->owner)
> +		module_put(bus->owner);
> +}

[Severity: High]
This is a pre-existing issue, but this balances the bus owner refcount
based on the old device's driver owner.

In the case where phy_attach_direct() fails due to the PHY already being
attached, this module_put() executes with the old caller's module owner.
Could this lead to module refcount underflows or leaks because it balances
the new caller's try_module_get() using the old caller's module?

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