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

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

[Aleksei Sviridkin]

phy_remove() clears phydev->drv as its last act; the driver core
clears d->driver only afterwards, in device_unbind_cleanup(). In that
window phy_attach_direct() skips the genphy substitution, because
d->driver is still set, and then dereferences the NULL phydev->drv in
phy_drv_supports_irq().

Refuse the attach there, before any reference on the driver is taken.
The function holds no lock over phydev->drv, and it cannot hold
device_lock across the attach: for a genphy-substituted PHY its error
path reaches device_release_driver() on the same device, which takes
that lock again. So this closes the case where the unbind is already
in flight; an unbind starting mid-attach still races.

Failing beats falling back to polling: phylink_bringup_phy()
dereferences phy->drv right after a successful attach, and 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. -ENODEV is wrong: DSA takes it as permission to
look for the PHY on the switch's internal MDIO bus.

Fixes: 61c81872815f ("net: phy: phy_device: Prevent nullptr exceptions on ISR")
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---

Notes:
    Found by reading the unbind path, not from a crash report: phy_remove()
    clears phydev->drv before the driver core clears d->driver, while
    phy_attach_direct() keys its genphy substitution off d->driver.
    
    Verified on an MT7981 board (mtk_eth_soc GMAC, "MediaTek MT7981 PHY" at
    mdio-bus:00), 6.18.44, with a 200 ms msleep() added at the end of
    phy_remove() to hold the window open. Two images, identical except for
    this patch.
    
    Without the patch, backgrounding
    
      echo mdio-bus:00 > "/sys/bus/mdio_bus/drivers/MediaTek MT7981 PHY/unbind"
    
    and immediately running "ip link set wan up" oopses on the first
    attempt:
    
      Unable to handle kernel access to user memory outside uaccess
      routines at virtual address 0000000000000128
      pc : phy_attach_direct+0x150/0x380
      Call trace:
       phy_attach_direct+0x150/0x380 (P)
       mtk_open+0x38/0xb70
    
    x0 is 0 and 0x128 is the offset of config_intr in struct phy_driver.
    
    With the patch the same sequence fails the attach on the first attempt
    instead, "wan: mtk_open: could not attach PHY: -16", and no oops is
    logged. Binding the driver back and bringing the interface up afterwards
    succeeds with the link up, so the early return leaves the phydev
    reusable. An ordinary bring-up is unaffected, and with the driver left
    unbound the genphy substitution still runs: "PHY [mdio-bus:00] driver
    [Generic PHY]", link up.

 drivers/net/phy/phy_device.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 94b2e85e00a3..044cefd9840b 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;
 	}
 
 	if (!try_module_get(d->driver->owner)) {
-- 
2.53.0

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

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

[Maxime Chevallier]

Hi,

On 9/14/26 22:42, Aleksei Sviridkin wrote:
> phy_remove() clears phydev->drv as its last act; the driver core
> clears d->driver only afterwards, in device_unbind_cleanup(). In that
> window phy_attach_direct() skips the genphy substitution, because
> d->driver is still set, and then dereferences the NULL phydev->drv in
> phy_drv_supports_irq().

[...]

>     Verified on an MT7981 board (mtk_eth_soc GMAC, "MediaTek MT7981 PHY" at
>     mdio-bus:00), 6.18.44, with a 200 ms msleep() added at the end of
>     phy_remove() to hold the window open. Two images, identical except for
>     this patch.

If you need to add msleep(200) in the core of the framework to trigger that,
this is net-next material.

Maxime

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

[Aleksei Sviridkin]

Fine, v2 will target net-next.

Out of curiosity, since I will run into this again: is the line
"needs instrumentation to reproduce -> net-next" about the window
being unreachable in practice, or about nobody having reported it?
The crash itself came from a real board; the msleep() only made the
race deterministic instead of occasional.

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

[Maxime Chevallier]

Hi,

On 9/17/26 20:33, Aleksei Sviridkin wrote:
> Fine, v2 will target net-next.
> 
> Out of curiosity, since I will run into this again: is the line
> "needs instrumentation to reproduce -> net-next" about the window
> being unreachable in practice, or about nobody having reported it?

Well there are lots of patches recently that fix bugs that are never
seen, and only show-up if some oddball error path is taken, under
conditions almost impossible to encounter in real life or even
reproduce without instrumenting the code to force these conditions.

> The crash itself came from a real board; the msleep() only made the
> race deterministic instead of occasional.

This is something you should say in the commit log then, it's important
to know this was seen in real life. If that's the case, then yes it's
'net' material instead of 'net-next'.

Maxime

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

[Aleksei Sviridkin]

I need to correct what I wrote earlier, because it decides the tree.

What was seen on hardware, with no instrumentation, is the class of
bug: an MT7981 board running an OpenWrt 6.18 kernel, with the distro's
backports and local patches plus the series I was testing on top,
oopsed twice on a NULL phydev->drv after a sysfs unbind of the PHY
driver, once inside a running phy_attach_direct() (in the driver's
config_init) and once in the PHY state machine. I was unbinding on
purpose, racing it against port teardown and bring-up, to stress that
series; none of this shows up in normal operation.

The dereference this patch prevents is a narrower member of that
family: drv already NULL when the attach starts. That window is the
short stretch between phy_remove()'s last store and
device_unbind_cleanup(), and the board never hit it on its own; the
msleep() was needed to reach it at all. So "the msleep() only made the
race deterministic" in my previous mail was wrong. The patch closes the
entry to that window; the wider race, an unbind landing mid-attach, is
untouched.

I will put all of that in the commit log. With both facts on the table,
net with the Fixes tag or net-next is your call.