Re: [PATCH net-next v4 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 1 potential
issue(s) to consider.

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

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] In phy_attach_direct() (drivers/net/phy/phy_device.c), this
  patch adds `phydev->drv_owner = d->driver->owner;` at line 1834, right…

--

net: phy: put the driver module the attach took

This commit adds a drv_owner field to struct phy_device. The field
records which PHY driver module phy_attach_direct() pinned. On detach,
phy_detach_internal() and the attach error path now release that
recorded module instead of reading phydev->mdio.dev.driver again.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 0bdd2dc84d81a..b5074599c988f 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1831,6 +1831,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  		err = -EIO;
>  		goto error_put_device;
>  	}
> +	phydev->drv_owner = d->driver->owner;

[Severity: Medium]
Can this record a different module than the one just pinned? The
reference is taken from one load of d->driver, and the owner is saved
from a second, separate load:

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

At this commit, nothing in phy_attach_direct() blocks a driver core
unbind. A sysfs unbind or a module removal runs phy_remove() and then
device_unbind_cleanup(), which calls device_set_driver(dev, NULL).

Suppose that happens between the two loads:

CPU0 (attach, under RTNL)          CPU1 (unbind_store())
try_module_get(d->driver->owner)
                                   phy_remove()
                                   device_unbind_cleanup()
                                     device_set_driver(dev, NULL)
phydev->drv_owner = d->driver->owner

Would the second load then dereference a NULL d->driver? Before this
patch, the non-genphy path did not read d->driver a second time here,
and the read on the detach side checked for NULL.

Now suppose a different driver binds in that window instead. Then
drv_owner names a module that was never pinned. phy_detach_internal()
would call module_put() on that module, and the reference on the
original module would leak. That is the same mismatch this patch is
meant to remove.

The later patch in the series, "net: phy: serialise attach and detach
with PHY driver bind and unbind", closes the unbind window. It holds
phydev->bind_lock across both loads.

The genphy path still has a gap. When d->driver started out NULL, a
concurrent really_probe() does not take bind_lock, so it could still
change d->driver between the two loads.

Would it be simpler to load the owner once into a local variable? Then
the recorded module would always be the one that was pinned:

	struct module *owner = d->driver->owner;

	if (!try_module_get(owner)) {
		...
	}
	phydev->drv_owner = owner;

>  
>  	if (phydev->is_genphy_driven) {
>  		err = d->driver->probe(d);

[ ... ]

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