Re: [PATCH net] net: phy: reject attach while the PHY driver is in transition

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 0

- [High] Incomplete fix / residual TOCTOU: the new `else if
  (!phydev->drv)` test in phy_attach_direct()…
- [Medium] Module reference imbalance survives the residual unbind window
  that this patch claims to close.

--

net: phy: reject attach while the PHY driver is in transition

The patch adds an early -EBUSY return in phy_attach_direct() when
d->driver is still set but phydev->drv has already been cleared, which is
the window between phy_remove() and device_unbind_cleanup(). The intent is
to avoid dereferencing a NULL phydev->drv in phy_drv_supports_irq() and to
avoid leaking the PHY driver module reference.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a37..044cefd9840bb 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1781,6 +1781,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  			d->driver = &genphy_driver.mdiodrv.driver;
>  
>  		phydev->is_genphy_driven = 1;
> +	} else if (!phydev->drv) {
> +		/* d->driver outlives phydev->drv on unbind, precedes it on bind */
> +		err = -EBUSY;
> +		goto error_put_device;
>  	}

[Severity: High]
Is this check able to prevent the oops quoted in the changelog, given that
it samples phydev->drv once and the dereference happens much later in the
same function?

The commit message says the function holds no lock over phydev->drv, and
get_device(d) only pins the device object, so the unbind can land right
after the test:

CPU0 phy_attach_direct()
	} else if (!phydev->drv) {	/* passes, drv still set */

CPU1 phy_remove()
	phydev->drv = NULL;

CPU0 continues in phy_attach_direct()
	if (!phy_drv_supports_irq(phydev->drv) && phy_interrupt_is_valid(phydev))

and phy_drv_supports_irq() has no NULL test:

static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
{
	return phydrv->config_intr && phydrv->handle_interrupt;
}

That is the same faulting access described in the notes (offset 0x128 being
phy_driver::config_intr), and it is the dereference named by the Fixes: tag.
The changelog does acknowledge "an unbind starting mid-attach still races",
but with a Fixes: tag attached, would a stable backporter reasonably
conclude the crash signature is gone when it is still reachable?

Should the single unguarded use be made NULL-tolerant instead, the way
phy_init_hw() already is, rather than adding a sampled check far away from
it?

There is a second dereference in the same window. If
device_unbind_cleanup() also runs, dev->driver becomes NULL:

drivers/base/dd.c:device_unbind_cleanup() {
	...
	device_set_driver(dev, NULL);
	...
}

and the statement immediately after the new check re-reads it:

	if (!try_module_get(d->driver->owner)) {

Can that path fault too, since the new test looks at phydev->drv but this
line dereferences d->driver?

Finally, phylink_bringup_phy() dereferences phy->drv right after a
successful attach. If the unbind stores land just as phy_attach_direct()
returns 0, does that caller hit a NULL phy->drv as well?

[Severity: Medium]
The changelog gives the module reference leak as a reason to fail here:

  "a continued attach would already hold the driver module reference that
  phy_detach() drops only while d->driver is set, leaking it once the
  unbind completes"

Does that leak remain reachable in the window the patch leaves open?
phy_attach_direct() charges the reference keyed on the mutable field, right
after the new check:

	if (!try_module_get(d->driver->owner)) {

and phy_detach() re-reads the same field at release time instead of the
module that was charged:

	if (phydev->mdio.dev.driver)
		module_put(phydev->mdio.dev.driver->owner);

So for the sequence [new check passes] -> [try_module_get(A->owner)] ->
[phy_remove() clears phydev->drv, device_unbind_cleanup() clears
dev->driver] -> [attach fails later, for example on the phydev->attached_dev
"PHY already attached" path, phy_link_topo_add_phy() or phy_init_hw(), or
the netdev is brought down afterwards], phy_detach() sees a NULL
d->driver and skips the module_put(). Does module A then stay pinned
forever?

There is also a mismatched-module variant: if the mdio device is bound to a
different PHY driver B via sysfs before phy_detach() runs, phy_detach()
would call module_put(B->owner) for a reference it never took. Can that
underflow B's refcount and allow unloading a module still in use?

Would recording the module pinned at attach time, rather than sampling
d->driver twice, close both cases?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914204200.2743251-1-f%40lex.la