Re: [PATCH 03/17] net: phy: Attach generic PHY drivers through the driver core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] net: phy: -EBUSY conflation and phydev->irq race in
  phy_attach_direct()
- [High] net: phy: unlocked d->driver check in phy_attach_direct()
  bypasses trylock
- [High] net: phy: pinctrl_bind_pins() fails with -EINVAL on fixed-link
  PHYs
- [Medium] net: phy: phy_attach_direct() leaks -EPROBE_DEFER to
  non-probe callers
--

commit 041c1bf498be3c0bd71084280b91acccb80528b3
Author: Leon Romanovsky <leon@kernel.org>

net: phy: Attach generic PHY drivers through the driver core

This commit introduces device_driver_try_attach() in the driver core and
updates phy_attach_direct() to attach the generic PHY fallback drivers
through the driver core rather than calling probe() and device_bind_driver()
directly.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 5d4508bcfbe99..8cc0de755bd08 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1912,7 +1912,9 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	struct device *d = &phydev->mdio.dev;
>  	struct module *ndev_owner = NULL;
>  	struct module *drv_owner;
> +	struct device_driver *drv;
>  	int irq = phydev->irq;
> +	bool bind_genphy;
>  	int err;
>  
>  	/* For Ethernet device drivers that register their own MDIO bus, we
> @@ -1939,16 +1941,16 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	/* Assume that if there is no driver, that it doesn't
>  	 * exist, and we should use the genphy driver.
>  	 */
> -	if (!d->driver) {
> +	drv = d->driver;
> +	bind_genphy = !drv;
> +	if (bind_genphy) {
>  		if (phydev->is_c45)
> -			d->driver = &genphy_c45_driver.mdiodrv.driver;
> +			drv = &genphy_c45_driver.mdiodrv.driver;
>  		else
> -			d->driver = &genphy_driver.mdiodrv.driver;
> -
> -		phydev->is_genphy_driven = 1;
> +			drv = &genphy_driver.mdiodrv.driver;
>  	}

[Severity: High]
Does checking d->driver without holding device_lock(d) or checking
device_is_bound(d) bypass device_driver_try_attach() when a specific PHY
driver is concurrently probing?

In drivers/base/dd.c, really_probe() sets dev->driver at the very start of
probing via device_set_driver(), before running pinctrl_bind_pins(),
driver_sysfs_add(), and call_driver_probe() -> phy_probe():

drivers/base/dd.c:really_probe() {
    re_probe:
        device_set_driver(dev, drv);

        /* If using pinctrl, bind pins now before probing */
        ret = pinctrl_bind_pins(dev);
        ...
        ret = call_driver_probe(dev, drv);
}

Because d->driver is already non-NULL throughout the entire execution of
really_probe() and phy_probe() (including when phy_probe() ->
phy_setup_ports() -> phy_sfp_add_upstream() -> sfp_bus_add_upstream()
blocks waiting for RTNL with device_lock(d) held), a concurrent call to
phy_attach_direct() under RTNL sees drv != NULL, sets bind_genphy to false,
and skips device_driver_try_attach() altogether.

When that happens, phy_attach_direct() races with the in-progress probe (or
an in-progress unbind in __device_release_driver()):

1. If the concurrent thread is in really_probe() after device_set_driver()
   but before phy_probe() assigns phydev->drv = phydrv, or in
   __device_release_driver() after phy_remove() clears phydev->drv = NULL
   before device_unbind_cleanup() clears d->driver, phy_attach_direct()
   passes a NULL phydev->drv to phy_drv_supports_irq(phydev->drv) and
   dereferences NULL.

2. If phy_probe() is blocked waiting for RTNL in sfp_bus_add_upstream(),
   phy_attach_direct() runs phy_init_hw() and phy_resume() on a half-probed
   PHY before phy_probe() has finished setting up SFP ports,
   phy_advertise_supported(), EEE, pause bits, LEDs, or phydev->state =
   PHY_READY. Once RTNL is released, phy_probe() overwrites
   phydev->advertising and resets phydev->state to PHY_READY.

3. If the concurrent really_probe() or phy_probe() later fails, it asserts
   hardware reset via phy_device_reset(phydev, 1) and runs
   device_unbind_cleanup() -> devres_release_all(), freeing devm-allocated
   phydev->priv while the net_device remains attached to phydev.

Conversely, if d->driver is NULL when read into drv, and a concurrent
specific-driver probe finishes and releases device_lock(d) before
device_driver_try_attach(drv, d) runs, __driver_probe_device() sees
dev->driver != NULL and returns -EBUSY, causing phy_attach_direct() to fail
with -EPROBE_DEFER instead of using the newly bound specific driver.

>  
> -	drv_owner = d->driver->owner;
> +	drv_owner = drv->owner;
>  	if (!try_module_get(drv_owner)) {
>  		phydev_err(phydev, "failed to get the device driver module\n");
>  		err = -EIO;
> @@ -1956,11 +1958,20 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	}
>  	phydev->drv_owner = drv_owner;
>  
> -	if (phydev->is_genphy_driven) {
> -		err = d->driver->probe(d);
> -		if (err >= 0)
> -			err = device_bind_driver(d);
> -
> +	if (bind_genphy) {
> +		/*
> +		 * RTNL is held, and a concurrent bind of a specific driver may
> +		 * wait for it with the device lock held. Never wait for that
> +		 * lock, and let the caller retry.
> +		 */
> +		err = device_driver_try_attach(drv, d);

[Severity: High]
What happens on Device Tree platforms with fixed-link PHYs when
device_driver_try_attach() routes generic PHY attachment through
really_probe() -> pinctrl_bind_pins()?

When a MAC driver registers a fixed-link PHY via
of_phy_register_fixed_link(np) or of_phy_get_and_connect(),
fixed_phy_register() in drivers/net/phy/fixed_phy.c assigns the MAC
controller's DT node to phy->mdio.dev.of_node without setting
dev_set_of_node_reused(&phy->mdio.dev):

drivers/net/phy/fixed_phy.c:fixed_phy_register() {
    of_node_get(np);
    phy->mdio.dev.of_node = np;
    phy->is_pseudo_fixed_link = true;
}

Because a fixed PHY has phy_id == 0, it does not match any driver during
phy_device_register() and always attaches to genphy_driver in
phy_attach_direct(). Previously, phy_attach_direct() called
d->driver->probe(d) and device_bind_driver(d), bypassing really_probe() and
pinctrl_bind_pins().

With device_driver_try_attach() calling __driver_probe_device() ->
really_probe() -> pinctrl_bind_pins(d), dev_of_node_reused(d) is false in
drivers/base/pinctrl.c:pinctrl_bind_pins(), so pinctrl attempts to claim the
MAC's pinctrl-0 pins a second time under the fixed PHY device. When the MAC
device has already claimed its pins, pin_request() in
drivers/pinctrl/pinmux.c rejects the duplicate mux claim with -EINVAL, which
pinctrl_bind_pins() propagates, causing phy_attach_direct() to fail with
-EINVAL on fixed-link interfaces whose MAC node defines pinctrl-0.

> +		if (err == -EBUSY) {
> +			/* Another bind owns the PHY, so nothing was probed here. */
> +			irq = phydev->irq;
> +			err = -EPROBE_DEFER;
> +		} else if (err == -EAGAIN) {
> +			err = -EPROBE_DEFER;
> +		}
>  		if (err)
>  			goto error_module_put;
>  	}

[Severity: Medium]
Can returning -EPROBE_DEFER from phy_attach_direct() leak to userspace or
cause permanent probe failures for callers that do not handle deferral?

Most Ethernet drivers that call phy_attach_direct() with RTNL held (via
phylink_of_phy_connect(), phylink_fwnode_phy_connect(),
phylink_connect_phy(), phy_connect(), or phy_connect_direct()) do so from
their net_device_ops->ndo_open() callback during __dev_open() (triggered by
SIOCSIFFLAGS or RTM_NEWLINK), rather than from a driver-core probe callback.
__dev_open() does not retry on -EPROBE_DEFER; it aborts opening the interface
and returns -EPROBE_DEFER (-517) directly to userspace.

Additionally, drivers that attach PHYs during .probe() via of_phy_connect()
in drivers/net/mdio/of_mdio.c only receive NULL when phy_connect_direct()
returns an error, losing the -EPROBE_DEFER errno. And because
__device_driver_attach() calls __driver_probe_device() instead of
driver_probe_device(), a supplier deferral (-EPROBE_DEFER converted to
-EAGAIN) does not add &phydev->mdio.dev to deferred_probe_pending_list.

> @@ -2071,8 +2082,6 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	module_put(phydev->drv_owner);
>  	phydev->drv_owner = NULL;
>  	phydev->irq = irq;

[Severity: High]
Could conflating all -EBUSY returns from device_driver_try_attach() with
lock contention corrupt phydev->irq in phy_attach_direct()?

In drivers/base/dd.c, __device_driver_attach() can return -EBUSY in four
different cases, not only when __device_driver_trylock() fails before
probing (which also makes the kernel-doc comment on
device_driver_try_attach() inaccurate):

1. __device_driver_trylock() fails because device_lock(dev) or
   device_lock(dev->parent) is contended.
2. __driver_probe_device() returns -EBUSY because dev->driver is already
   set.
3. really_probe() fails with -EBUSY when !list_empty(&dev->devres_head).
4. call_driver_probe() -> phy_probe() fails with -EBUSY (for example,
   phy_setup_ports() -> of_phy_ports() -> phy_add_port() returning -EBUSY,
   or an MDIO read in genphy_read_abilities(),
   genphy_c45_pma_read_abilities(), or genphy_c45_read_eee_adv() returning
   -EBUSY), because really_probe() negates negative probe errors to +EBUSY
   and __device_driver_attach() negates ret > 0 back to -EBUSY:

drivers/base/dd.c:__device_driver_attach() {
    ret = __driver_probe_device(drv, dev);
    __device_driver_unlock(dev, dev->parent);

    /* also return probe errors as normal negative errnos */
    if (ret > 0)
        ret = -ret;
}

When phy_probe() runs for genphy_driver or genphy_c45_driver and then fails
with -EBUSY, phy_probe() has already overwritten phydev->irq with PHY_POLL:

drivers/net/phy/phy_device.c:phy_probe() {
    if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev))
        phydev->irq = PHY_POLL;
}

In phy_attach_direct(), the err == -EBUSY branch assumes nothing was probed,
reads irq = phydev->irq (which is now PHY_POLL), converts the error to
-EPROBE_DEFER, and writes phydev->irq = irq (PHY_POLL) at error_module_put,
permanently losing the PHY's original hardware interrupt number.

Conversely, when device_driver_try_attach() returns -EBUSY because another
thread holds device_lock(d) inside phy_probe(), phy_attach_direct() reads
irq = phydev->irq and writes phydev->irq = irq at error_module_put without
holding device_lock(d). If irq = phydev->irq is read before the concurrent
phy_probe() sets phydev->irq = PHY_POLL, and phydev->irq = irq is written at
error_module_put after phy_probe() sets PHY_POLL, phy_attach_direct()
overwrites the concurrent driver's PHY_POLL setting with the stale IRQ
number. Similarly, if device_driver_try_attach() fails with -EAGAIN before
phy_probe() runs, error_module_put writes phydev->irq = irq after
__device_driver_unlock() has already released device_lock(d).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-device-trust-core-v1-0-1e536825b4eb@nvidia.com?part=3