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

[netdev-bot+sashiko@kernel.org]

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