[PATCH net-next v2] net: phy: refuse to attach a PHY whose driver is being unbound

[PATCH net-next v2] net: phy: refuse to attach a PHY whose driver is being unbound

[Aleksei Sviridkin]

phy_remove() clears phydev->drv as its last act; the driver core clears
d->driver only afterwards, in device_unbind_cleanup(). An attach entering
that window still sees d->driver, so it skips the genphy substitution and
then dereferences the NULL phydev->drv.

Unbinding a PHY driver under an attached consumer crashes in real life.
The board that showed it is an MT7981 whose copper PHY driver needs
firmware from the rootfs, so the driver arrives after DSA has attached
the PHY. I was testing how a port copes with that driver coming and
going, on an OpenWrt 6.18 kernel with the distro's backports, local
patches and the series under test. Racing a sysfs unbind against port
teardown and bring-up oopsed twice: once in the state machine, and once
inside a live phy_attach_direct() where phy_init_hw() had already
entered the driver's config_init(). The second came after a
one-line NULL check at the state-machine site let the run continue, with
nothing added to widen the window. Both have this root. Neither is this
dereference: in one the driver went away under a port close, in the other
mid-attach, and in neither was phydev->drv already NULL when an attach
started. That window is the narrow member of the family, and reaching it on
demand needed a 200 ms msleep() at the end of phy_remove().

Refuse the attach rather than let it complete on a driverless PHY, with
-EBUSY, which this function already returns when the PHY is attached
elsewhere. Without phylink the netdev would come up on a PHY that never ran
config_init. Under phylink it does not get that far: phylink_bringup_phy()
dereferences phy->drv->name as soon as the attach returns.

The mid-attach case needs serialisation rather than a NULL test.
phy_init_hw() tests phydev->drv once on entry and then dereferences it
several more times, and it calls the driver's own config_init(), which is
where one of those oopses landed, on a dereference made by driver code that
no test in phylib can reach. The device lock is not available either:
phy_attach_direct() runs under rtnl from ndo_open, while phy_remove() runs
under the device lock and calls sfp_bus_del_upstream(), which takes rtnl
for a PHY with an SFP bus.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---

Notes:
    v1: https://lore.kernel.org/netdev/20260914204200.2743251-1-f@lex.la/
    
    v2:
    - Said in the commit message where this was seen. The site in
      phy_attach_direct() was found by reading the unbind path, but the race it
      belongs to was not theoretical: it took the same board down twice with no
      instrumentation in the kernel, while a sysfs unbind of the PHY driver was
      raced against port teardown and bring-up. v1 opened with "found by
      reading, not from a crash report", which was true of the site and
      misleading about the race; that line is gone. The second of those two
      came after the state-machine site had been given a local NULL test so the
      run could continue, so the kernel that produced it carried that one extra
      check; nothing was added to widen the window in either. The traces were
      read at the time and the dumps were not preserved.
    - Tree and Fixes: tag follow Maxime Chevallier's reading of how reachable
      this is in practice:
      https://lore.kernel.org/netdev/6e82dc04-a68d-4c54-a6f0-a13c04fa2eef@bootlin.com/
      https://lore.kernel.org/netdev/65b2eff4-6818-4cfa-a0e7-d48729e0cd9e@bootlin.com/
      and the answer that separates the two windows:
      https://lore.kernel.org/netdev/20260917210406.1651902-1-f@lex.la/
    - Kept the guard instead of making phy_drv_supports_irq() NULL-tolerant. A
      NULL test there moves the fault to the caller rather than removing it:
      phylink_bringup_phy() dereferences phy->drv->name as soon as the attach
      returns. phylink is inconsistent about this on its own:
      phylink_sfp_connect_phy() refuses a PHY with no driver and
      phylink_bringup_phy() does not, which is an argument for the check living
      in phylib, where every caller gets it.
    - The module reference charged on d->driver->owner by phy_attach_direct()
      and released by phy_detach() re-reading the same field is mispaired
      whenever that field has moved in between: the put is skipped if the
      driver is still unbound at detach time, and goes to a module that was
      never charged if a different driver was bound meanwhile. An unbind
      followed by a rebind of the same driver balances, which is why an
      ordinary cycle shows nothing. Separate defect, separate patch.
    
    Verified on a Netcraze NC-1012 (MT7981) running OpenWrt 6.18.44, with a
    200 ms msleep() added at the end of phy_remove() to hold the window open.
    Two images off one tree, 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" faults:
    
      Unable to handle kernel access to user memory outside uaccess routines
      at virtual address 0000000000000128
      CPU: 1 PID: 4487 Comm: ip
      pc : phy_attach_direct+0x150/0x380
      x0 : 0000000000000000
      Call trace:
       phy_attach_direct+0x150/0x380 (P)
       phylink_fwnode_phy_connect+0x198/0x27c
       phylink_of_phy_connect+0x18/0x20
       mtk_open+0x38/0xb70
      Kernel panic - not syncing: Oops: Fatal exception
    
    0x128 is the offset of config_intr in struct phy_driver in this build,
    read out of the disassembly of phy_attach_direct(): the inlined
    phy_drv_supports_irq() is "ldr x1, [x0, #296]" followed by
    "ldr x0, [x0, #304]". This board has panic_on_oops set, so the kernel
    panicked and rebooted, and that erased the test log (the run script still
    wrote to /tmp for this image; it was moved to /root before the patched
    run). How many attempts it took was therefore not preserved, and the trace
    above is what pstore kept.
    
    With the patch the same sequence fails the attach on the first attempt
    instead, with no oops: "ip: SIOCSIFFLAGS: Resource busy" from the ioctl,
    and "wan: mtk_open: could not attach PHY: -16" in the log.
    Binding the driver back and bringing the interface up afterwards links, so
    the refusal leaves the phydev reusable. An ordinary bring-up still attaches
    the driver, and with the driver left unbound the genphy substitution still
    runs:
    "PHY [mdio-bus:00] driver [Generic PHY] (irq=POLL)", link up at 1Gbps.

 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 41d1b20cde2c..f13c9d7fd47f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1908,6 +1908,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-next v2] net: phy: refuse to attach a PHY whose driver is being unbound

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new `else if (!phydev->drv)` guard in phy_attach_direct()
  (drivers/net/phy/phy_device.c:1821) is a single unsynchronized plain…
- [Low] The kernel-doc block above phy_attach_direct()
  (drivers/net/phy/phy_device.c:1781-1788) still states unconditionally…
- [Low] Submission metadata is incomplete and self-inconsistent for a
  crash fix: the commit fixes a reproducible NULL-pointer…

Pre-existing issues:
- [Medium] Module reference mispairing across unbind: phy_attach_direct()
  charges the reference with try_module_get(d->driver->owner)…

--

net: phy: refuse to attach a PHY whose driver is being unbound

phy_attach_direct() gains a new arm that refuses the attach with -EBUSY
when the mdio device still has a d->driver but phydev->drv has already
been cleared by phy_remove(). The intent is to avoid completing an attach
on a driverless PHY, where the NULL phydev->drv is later dereferenced.

A few questions below about the new check and its neighbourhood.

[Severity: Low]
The changelog says:

  "Tree and Fixes: tag follow Maxime Chevallier's reading of how reachable
  this is in practice"

but the commit carries only Assisted-by: and Signed-off-by: trailers, with
no Fixes: tag, no Cc: stable and no target tree in the subject prefix.

Documentation/process/maintainer-netdev.rst says:

   - for fixes the ``Fixes:`` tag is required, regardless of the tree

Since this is described as fixing a reproducible oops, could the Fixes: tag
(and a stable designation, if one is intended) be added, pointing at the
commit that introduced the phydev->drv = NULL ordering in phy_remove()?

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 5b13a74e2fa9..dfc2b29cac8a 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1818,6 +1818,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]
Does this check actually close the window described in the commit message?
It is a single unlocked read of phydev->drv, and nothing keeps the observed
value valid for the rest of phy_attach_direct().

phy_remove() stores the NULL as its last statement, holding only the device
lock:

  drivers/net/phy/phy_device.c:phy_remove() {
	...
	phydev->drv = NULL;
  }

phy_attach_direct() runs under rtnl and takes neither device_lock(d) nor
phydev->lock around the new test, so the store can land at any point after
the test. After the test the attach does sleeping work
(device_bind_driver(), phy_link_topo_add_phy(), sysfs link creation) and
then re-reads the pointer:

  if (!phy_drv_supports_irq(phydev->drv) && phy_interrupt_is_valid(phydev))
	  phydev->irq = PHY_POLL;

with

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

which dereferences the pointer unconditionally. Isn't that the same
instruction pair quoted in the commit message, the config_intr/handle_interrupt
loads at phy_attach_direct+0x150?

phy_init_hw(), called later in the same function, has the same shape: one
test on entry followed by several more dereferences and a call into driver
code:

  drivers/net/phy/phy_device.c:phy_init_hw() {
	if (!phydev->drv)
		return 0;

	if (phydev->drv->soft_reset) {
	...
	if (phydev->drv->config_init) {
		ret = phydev->drv->config_init(phydev);

There is also a variant that needs no NULL at all: once phy_remove() has
run phy_sfp_release() and drv->remove() but has not yet reached
phydev->drv = NULL, the new test passes and the attach goes on to call into
a driver instance whose remove() has already completed.

The commit message acknowledges this ("The mid-attach case needs
serialisation rather than a NULL test"), but the message also opens with
"Unbinding a PHY driver under an attached consumer crashes in real life"
and quotes the phy_drv_supports_irq() fault as the motivating oops. Would
it be better to serialise the read with its uses against phy_remove() (a
phylib-level mutex taken by both paths, holding a driver reference, or
having unbind refuse or wait while attached_dev is set) instead of adding
another entry-point test, so that the reported faulting site is actually
covered?

[Severity: Low]
Should the kernel-doc above phy_attach_direct() be updated too? It still
promises the opposite for this exact case:

  * Description: Called by drivers to attach to a particular PHY
  *     device. The phy_device is found, and properly hooked up
  *     to the phy_driver.  If no driver is attached, then a
  *     generic driver is used.

and there is no Return: section listing the new refusal.

Also, -EBUSY is now returned from two places in this function with
unrelated meanings. The existing one is permanent and prints something:

	if (phydev->attached_dev) {
		dev_err(&dev->dev, "PHY already attached\n");
		err = -EBUSY;
		goto error;
	}

The new one is a transient bind/unbind window and is silent, so a bare
EBUSY from SIOCSIFFLAGS with an empty dmesg is indistinguishable from "this
PHY belongs to another interface". The in-tree counterpart for a driverless
PHY does log:

  drivers/net/phy/phylink.c:phylink_sfp_connect_phy() {
	if (!phy->drv) {
		phylink_err(pl, "PHY %s (id 0x%.8lx) has no driver loaded\n",
			    phydev_name(phy), (unsigned long)phy->phy_id);
		...
		return -EINVAL;
	}

Would a phydev_err() here, and/or a distinct errno, make the two cases
tellable apart?

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

[Severity: Medium]
This isn't a bug introduced by this patch, and the changelog already flags
it as a "Separate defect, separate patch", but noting it here since the new
arm sits directly above the try_module_get(): the reference charged on
d->driver->owner at attach time is released by re-reading the field at
detach:

  drivers/net/phy/phy_device.c:phy_detach_internal() {
	...
	if (phydev->mdio.dev.driver)
		module_put(phydev->mdio.dev.driver->owner);

If the PHY driver is unbound while the consumer is still attached, the
driver core clears the field in device_unbind_cleanup():

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

so the later test is false, module_put() is skipped, and the charged
reference is leaked, leaving that PHY driver module permanently unloadable.
If a different driver was bound in between, the put goes to a module that
was never charged; module_put() uses atomic_dec_if_positive() with
WARN_ON(), so this warns rather than corrupting memory, but the target
module ends up below MODULE_REF_BASE and unusable for future
try_module_get() calls. Is recording the module the reference was taken on
(rather than re-reading d->driver) the direction you have in mind for that
follow-up?

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