Re: [PATCH net-next v5 2/4] net: phy: put the driver module the attach took

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [Medium] At this commit, `phydev->drv_owner` is read and written with
  no lock.
- [Low] The commit body does not say what this patch depends on.

Pre-existing issues:
- [Medium] In `phy_attach_direct()` (drivers/net/phy/phy_device.c),
  `d->driver` is tested at line 1825 (`if (!d->driver)`) and…

--

net: phy: put the driver module the attach took

This commit stores the PHY driver module pinned by phy_attach_direct() in
a new field, phydev->drv_owner. phy_detach_internal() and the attach error
path now release that stored module instead of reading d->driver again.
The goal is to stop skipping the put, or putting the wrong module, when
the PHY driver is unbound while the PHY is attached.

> Fixes: cafe8df8b9bc ("net: phy: Fix lack of reference count on PHY driver")

[Severity: Low]
Should the commit message say which other patches this one depends on?
Because of the Fixes: tag, stable tooling could pick up this patch by
itself. Its correctness seems to depend on two other patches in the
series:

  "net: phy: refuse a second attach before touching the PHY", which needs
  to be applied first.

  "net: phy: serialise attach and detach with PHY driver bind and
  unbind", which closes a leak this patch leaves open (see below).

The only place this is mentioned is the v5 changelog below the --- line:

  "On its own, this patch can still leak a reference when one consumer
  attaches while another detaches the same PHY. Patch 3 closes that."

That text is dropped when the patch is applied.

Without the first patch, the baseline phy_attach_direct() still reaches
the drv_owner store on a PHY that is already attached. Its
if (phydev->attached_dev) test only runs after try_module_get():

phy_attach_direct()      /* second attach, same PHY */
    try_module_get(A);
    phydev->drv_owner = A;
    if (phydev->attached_dev) goto error;
    phy_detach_internal()
        module_put(phydev->drv_owner);    /* puts A */
        phydev->drv_owner = NULL;

When the first consumer later calls phy_detach(), it does
module_put(NULL), so A is left with an extra reference that is never
dropped. The baseline also allows a second attach without a netdev
(DSA-style), and that overwrites the slot in the same way.

This patch's hunks don't overlap the first patch's hunks, so it applies
cleanly without it. Could the body name the companion commits, or could
the series be reordered or squashed?

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 6bc9cdb7fa699..68691c487eee5 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1728,8 +1728,8 @@ static void phy_detach_internal(struct phy_device *phydev, bool notify_bus)
>  	phydev->phy_link_change = NULL;
>  	phydev->phylink = NULL;
>  
> -	if (phydev->mdio.dev.driver)
> -		module_put(phydev->mdio.dev.driver->owner);
> +	module_put(phydev->drv_owner);
> +	phydev->drv_owner = NULL;

[Severity: Medium]
Can a concurrent attach lose its module reference here, or have it put on
the wrong module? At this commit, phydev->drv_owner is read and written
without any lock.

Setting phydev->phy_link_change = NULL opens the unlocked "already
attached" test in phy_attach_direct() before this module_put() runs:

phy_attach_direct() {
    ...
	if (phydev->phy_link_change) {
		phydev_err(phydev, "PHY already attached\n");
    ...
	phydev->drv_owner = drv_owner;
    ...
}

So a second consumer that attaches in that window can store its owner in
the slot this detach is about to read and clear:

CPU1 (detach #1)                    CPU2 (attach #2)
phy_detach_internal()
  phydev->phy_link_change = NULL;
                                    phy_attach_direct()
                                      phy_link_change test passes
                                      try_module_get(X);
                                      phydev->drv_owner = X;
  module_put(phydev->drv_owner);
  phydev->drv_owner = NULL;

If X is the same driver A, detach #1 puts A and then its NULL store wipes
out attach #2's record. Attach #2's later detach puts NULL, so A leaks a
reference.

Now suppose A was unbound and driver B bound while the PHY was attached.
Detach #1 then puts B, which it never pinned, and A leaks. Attach #2 is
using B but holds no reference on it. Could B be unloaded while attach #2
is still using it?

If attach #2 then fails through goto error, phy_detach_internal() may
read drv_owner == B again and put B a second time. The error_module_put
path also reads phydev->drv_owner instead of the local drv_owner.

This looks like the "put lands on a module that was never pinned" case
the commit message describes, and the v5 changelog mentions the gap.

Later in the series, "net: phy: serialise attach and detach with PHY
driver bind and unbind" appears to close it:

  phy_attach_direct() holds bind_lock from before the phy_link_change
  test until after the drv_owner store.

  phy_detach_internal() clears phy_link_change, saves and clears
  drv_owner, and clears attached in a single bind_lock section, then
  calls module_put() on the saved value.

[ ... ]

> @@ -1830,11 +1831,13 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  		phydev->is_genphy_driven = 1;
>  	}
>  
> -	if (!try_module_get(d->driver->owner)) {
> +	drv_owner = d->driver->owner;

[Severity: Medium]
This isn't a bug introduced by this patch, but can d->driver change
between the earlier if (!d->driver) test in phy_attach_direct() and this
dereference?

phy_attach_direct() doesn't hold device_lock(d), and at this commit
nothing else blocks a driver core unbind. A sysfs unbind, or
driver_unregister() at module exit, can run at the same time:

__device_release_driver()
    phy_remove()
    dev->driver = NULL;

Could that cause a NULL dereference of ->owner here? Could it instead
read ->owner from the driver struct of a module that is being freed?

The old try_module_get(d->driver->owner) had the same check-then-
dereference pattern, so the race predates this patch.

"net: phy: serialise attach and detach with PHY driver bind and unbind",
later in the series, appears to close it. It holds phydev->bind_lock from
before the d->driver test through try_module_get(), and returns -EAGAIN
when d->driver is set but phydev->bound is not.

> +	if (!try_module_get(drv_owner)) {
>  		phydev_err(phydev, "failed to get the device driver module\n");
>  		err = -EIO;
>  		goto error_put_device;
>  	}
> +	phydev->drv_owner = drv_owner;
>  

[ ... ]

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