Re: [PATCH net-next v3 3/4] net: mdio: make device map changes hotplug-safe

[sashiko-bot@kernel.org]

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

New issues:
- [High] Race condition in `phy_claim()` causes concurrent MAC driver probes to fail with `-ENODEV` when attaching to a PHY.
- [Medium] Unbounded memory leak of dynamically removed PHY devices due to intentional pinning in `mdio_map_retired_phys`.

Pre-existing issues:
- [High] Missing NULL pointer check for `mdiobus_get_phy()` in `vsc8584_base_priv()` leads to kernel panic.
--

--- Patch [3]: [PATCH net-next v3 3/4] net: mdio: make device map changes hotplug-safe ---
commit d8ecb0a89add88ea9e28273a5be54ac5e3624bc8
Author: James Hilliard <james.hilliard1@gmail.com>

    net: mdio: make device map changes hotplug-safe
    
    MDIO device registration currently publishes directly into mdio_map
    without serializing address ownership, and removal frees the object
    immediately. That is sufficient while bus population is static, but
    dynamic firmware changes can race registration, PHY attachment and bus
    teardown. It also cannot safely preserve mdiobus_get_phy()'s
    borrowed-pointer convention.

> diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
> --- a/drivers/net/phy/mdio_device.c
> +++ b/drivers/net/phy/mdio_device.c

[ ... ]

> +int mdiobus_begin_remove(struct mdio_device *mdiodev, bool dynamic)
> +{
> +	struct mii_bus *bus = mdiodev->bus;
> +	struct phy_device *phydev = NULL;
> +	int err = 0;

[ ... ]

> +	if (dynamic && mdiodev->flags & MDIO_DEVICE_FLAG_PHY) {
> +		mdio_device_get(mdiodev);
> +		list_add_tail(&phydev->retired_node,
> +			      &bus->mdio_map_retired_phys);
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&bus->mdio_map_lock);
> +	return err;
> +}

[Severity: Medium]
Will this cause an unbounded memory leak if PHYs are dynamically added and
removed continuously?

When a PHY is dynamically removed, mdiobus_begin_remove() takes a reference
and adds the device to the mdio_map_retired_phys list. These pinned devices
are intentionally never freed until mdiobus_remove_devices() runs during the
teardown of the entire parent MDIO bus. Continuous application and removal of
device tree overlays could exhaust memory over time.

> diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
> --- a/drivers/net/phy/mscc/mscc_ptp.c
> +++ b/drivers/net/phy/mscc/mscc_ptp.c
> @@ -1279,10 +1279,8 @@ static struct vsc8531_private *vsc8584_base_priv(struct phy_device *phydev)
>  	struct vsc8531_private *vsc8531 = phydev->priv;
>  
>  	if (vsc8531->ts_base_addr != phydev->mdio.addr) {
> -		struct mdio_device *dev;
> -
> -		dev = phydev->mdio.bus->mdio_map[vsc8531->ts_base_addr];
> -		phydev = container_of(dev, struct phy_device, mdio);
> +		phydev = mdiobus_get_phy(phydev->mdio.bus,
> +					 vsc8531->ts_base_addr);
>  
>  		return phydev->priv;

[Severity: High]
This isn't a bug introduced by this patch, but could this result in a kernel
panic if the base PHY fails to initialize?

If the base PHY does not successfully initialize during MDIO bus scanning
(e.g., due to a hardware fault), mdiobus_get_phy() will return NULL. The
subsequent direct dereference of phydev->priv will crash.

>  	}
>  
>  	return vsc8531;
>  }

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1121,25 +1124,40 @@ int phy_device_register(struct phy_device *phydev)
>  	err = phy_scan_fixups(phydev);
>  	if (err) {
>  		phydev_err(phydev, "failed to initialize\n");
> -		goto out;
> +		return mdiobus_registration_done(&phydev->mdio, err);
>  	}
>  
>  	err = device_add(&phydev->mdio.dev);
> -	if (err) {
> +	if (err)
>  		phydev_err(phydev, "failed to add\n");
> -		goto out;
> -	}
>  
> -	return 0;
> +	return mdiobus_registration_done(&phydev->mdio, err);
>  }

[ ... ]

> +static int phy_claim(struct phy_device *phydev)
> +{
> +	struct mdio_device *mdiodev = &phydev->mdio;
> +	struct mii_bus *bus = mdiodev->bus;
> +	int err = 0;
> +
> +	mutex_lock(&bus->mdio_map_lock);
> +	if (bus->state != MDIOBUS_REGISTERED ||
> +	    bus->mdio_map_removing ||
> +	    bus->mdio_map[mdiodev->addr] != mdiodev ||
> +	    (bus->mdio_map_pending & BIT(mdiodev->addr)))
> +		err = -ENODEV;

[Severity: High]
Is it possible for a MAC driver probe to race with
mdiobus_registration_done() and fail permanently with -ENODEV?

In phy_device_register(), device_add() is called before
mdiobus_registration_done() completes the publication. Once device_add() is
called, the PHY becomes visible in the driver core's bus lists.

If a MAC driver concurrently probes the bus and finds the PHY, it will
attempt to attach. However, since mdiobus_registration_done() hasn't
executed yet, mdio_map_pending is still set for this address. The phy_claim()
check above will reject the valid attachment attempt with -ENODEV, causing
the MAC driver probe to permanently fail rather than deferring.

> +	else if (phydev->attached)
> +		err = -EBUSY;
> +	else
> +		phydev->attached = true;
> +	mutex_unlock(&bus->mdio_map_lock);
> +
> +	return err;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-submit-mdio-of-dynamic-v2-v3-0-16f3cc893b0e@gmail.com?part=3