Re: [PATCH net-next v4 1/4] net: phy: refuse a second attach before touching the PHY

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 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] The new 'already attached' guard in phy_attach_direct() is a
  lockless check-then-act.
- [Low] The commit message says the old error path ran
  phy_detach_internal() and, for the generic driver, unbound genphy.

--

net: phy: refuse a second attach before touching the PHY

This patch moves the "PHY already attached" test in phy_attach_direct()
to the start of the function, before any module or device references are
taken and before the generic driver is bound. It also tests
phydev->phy_link_change instead of phydev->attached_dev, so that a PHY
attached without a netdev is refused too.

> phy_attach_direct() tests phydev->attached_dev only after it has taken
> its references and, for a PHY with no driver, bound the generic one, and
> then leaves through the error path that calls phy_detach_internal().
> That call works on the PHY's current attachment, so a second attach of
> a PHY in use tears down the first consumer: it clears that netdev's
> phydev pointer and the PHY's attached_dev, removes the sysfs links,
> suspends the PHY and, for the generic driver, unbinds it, while the
> first consumer goes on using the PHY.

[Severity: Low]
Is this description accurate for a PHY that the generic driver drives?

Since commit 2796ff1e3dca ("net: phy: add flag is_genphy_driven to struct
phy_device"), is_genphy_driven is a field of struct phy_device and no
longer a local variable. On a second attach of a genphy-driven PHY,
d->driver is already set and is_genphy_driven is still 1, so the old code
runs:

phy_attach_direct() {
    ...
	if (phydev->is_genphy_driven) {
		err = d->driver->probe(d);
		if (err >= 0)
			err = device_bind_driver(d);

		if (err)
			goto error_module_put;
	}
    ...
}

That runs phy_probe() again on a PHY that is in use. It registers the LED
triggers a second time, resets the link modes and sets the state to
PHY_READY.

device_bind_driver() then fails in driver_sysfs_add() with -EEXIST, since
the links from the first bind are still there. The code then reaches:

error_module_put:
	module_put(d->driver->owner);
	phydev->is_genphy_driven = 0;
	d->driver = NULL;

At that point the first consumer still has the PHY bound and attached.

On this path, the old attached_dev test and phy_detach_internal() are
never reached. Could the message describe this failure mode instead? Does
the Fixes: tag cover how the problem appears on current kernels?

The removed dev_err(&dev->dev, ...) also dereferenced a pointer built from
a NULL dev. That happens when a PHY already attached to a netdev is
attached again without one. Switching to phydev_err() fixes that as well.
Is it worth mentioning in the message?

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 5b13a74e2fa92..0bdd2dc84d81a 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1794,6 +1794,12 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	struct module *ndev_owner = NULL;
>  	int err;
>  
> +	/* Set by every attach, with or without a netdev */
> +	if (phydev->phy_link_change) {
> +		phydev_err(phydev, "PHY already attached\n");
> +		return -EBUSY;
> +	}
> +

[Severity: Medium]
Can two concurrent attaches of the same phy_device both get past this
check?

phy_link_change is read here with no lock held. The claim is written much
later in phy_attach_direct():

	phydev->phy_link_change = phy_link_change;
	if (dev) {
		phydev->attached_dev = dev;
		dev->phydev = phydev;

Between the two are try_module_get(), get_device() and, for genphy,
d->driver->probe() and device_bind_driver(), which can sleep.

Not every caller holds RTNL. DSA attaches at probe time through
dsa_user_create()->dsa_user_phy_setup() before rtnl_lock(). CPU and DSA
link ports attach through
dsa_shared_port_phylink_register()->phylink_of_phy_connect(), also without
RTNL. MAC drivers attach from ndo_open with RTNL held.

So the following sequence looks possible:

CPU0                                 CPU1
phy_attach_direct()
  phy_link_change == NULL
                                     phy_attach_direct()
                                       phy_link_change == NULL
  phydev->phy_link_change = ...
  phydev->attached_dev = dev0
  dev0->phydev = phydev
                                       phydev->phy_link_change = ...
                                       phydev->attached_dev = dev1
                                       dev1->phydev = phydev
                                       phy_init_hw(phydev)

CPU1 overwrites attached_dev and phy_link_change and re-runs
phy_init_hw() on the PHY that dev0 is using, while dev0 keeps its phydev
pointer. For genphy, CPU1 re-runs the probe, device_bind_driver() fails,
and error_module_put clears d->driver and is_genphy_driven under the
first consumer.

The later patch in this series, "net: phy: serialise attach and detach
with PHY driver bind and unbind", adds phydev->bind_lock, but this check
stays outside it. An attacher that has passed the check blocks on
bind_lock while the first attach completes. It then takes the lock and
attaches again without checking a second time.

Would it make sense to do this test under bind_lock, or to claim the PHY
atomically, so the check and the assignment cannot be separated?

The reads and writes of phy_link_change here and in phy_detach_internal()
are also plain accesses without READ_ONCE()/WRITE_ONCE(). KCSAN would
probably report them as a data race.

[ ... ]

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