Re: [PATCH net-next v12 01/15] phylink: Add phylink_mac_interrupt

[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

- [Medium] `phylink_mac_interrupt()`
  (drivers/net/phy/phylink.c:1621-1628) reads `pl->phydev` with no…
- [Low] Claim that the newly exported `phylink_mac_interrupt()`
  (drivers/net/phy/phylink.c) and its prototype…

--

phylink: Add phylink_mac_interrupt

This adds an exported phylink_mac_interrupt() helper which forwards a
link-change interrupt received by the MAC to phylib by calling
phy_mac_interrupt() on pl->phydev.  It is intended for drivers that want to
avoid polling the PHY.

(no Link: tags in the commit header)

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 3ec3bb4391095..96ed13458e9f3 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1612,6 +1612,22 @@ static bool phylink_link_is_up(struct phylink *pl)
>  	return pl->netdev ? netif_carrier_ok(pl->netdev) : pl->old_link_state;
>  }
>  
> +/**
> + * phylink_mac_interrupt() - wrapper for phy_mac_interrupt()
> + * @pl: a pointer to a &struct phylink returned from phylink_create()
> + *
> + * Passes a link-change interrupt received by the MAC to phylib.
> + */
> +void phylink_mac_interrupt(struct phylink *pl)
> +{
> +	struct phy_device *phy;
> +
> +	phy = pl->phydev;
> +	if (phy)
> +		phy_mac_interrupt(phy);
> +}
> +EXPORT_SYMBOL_GPL(phylink_mac_interrupt);

[Severity: Medium]
Is the unsynchronized read of pl->phydev here safe?  Every other reader of
that field in phylink.c is serialized, and the struct member carries the
comment describing that rule:

	/* Serialize updates to pl->phydev with phylink_resolve() */
	struct mutex phydev_mutex;

phylink_resolve() and phylink_bringup_phy() take pl->phydev_mutex, and the
ethtool/ioctl/phylink_speed_up()/phylink_speed_down() readers run under
ASSERT_RTNL().  Since this new helper is documented as being called from a
MAC link-change interrupt, it can take neither the mutex nor RTNL, and the
signature (struct phylink * only) gives the caller no way to pin the
phy_device across the call.

Can this race with teardown?  phylink_disconnect_phy() is the only writer
that clears the pointer:

drivers/net/phy/phylink.c:phylink_disconnect_phy() {
	mutex_lock(&pl->phydev_mutex);
	phy = pl->phydev;
	if (phy) {
		...
		pl->phydev = NULL;
		...
	}
	mutex_unlock(&pl->phydev_mutex);

	if (phy) {
		flush_work(&pl->resolve);
		phy_disconnect(phy);
	}
}

So the sequence would be:

CPU0 (MAC link IRQ)
phylink_mac_interrupt()
	phy = pl->phydev;		/* latched */

CPU1 (RTNL)
phylink_disconnect_phy()
	pl->phydev = NULL;
	phy_disconnect(phy)		/* phy_stop_machine() ->
					   cancel_delayed_work_sync(
						&phydev->state_queue) */

CPU0 resumes
	phy_mac_interrupt(phy)
		phy_trigger_machine(phy)
			phy_queue_state_machine()
				mod_delayed_work(..., &phy->state_queue, ...)

Does that re-arm the PHY state machine work after the teardown path already
did its synchronous cancel, so the state machine runs on a detached phydev?

For an SFP cage the phy_device is also freed while the interface is still
up:

drivers/net/phy/sfp.c:sfp_sm_phy_detach() {
	sfp_remove_phy(sfp->sfp_bus);	/* -> phylink_sfp_disconnect_phy()
					   -> phylink_disconnect_phy() */
	phy_device_remove(sfp->mod_phy);
	phy_device_free(sfp->mod_phy);
	sfp->mod_phy = NULL;
}

Can the mod_delayed_work() above then write into the freed phy_device?

Separately, the load of pl->phydev is a plain access to a field written
under a mutex on another CPU.  Should this at least be READ_ONCE()?  As
written, nothing prevents the compiler from re-loading pl->phydev when
forming the call argument, which would make the "if (phy)" test useless and
pass NULL into phy_mac_interrupt() -> mod_delayed_work().

Would it make sense to either make pl->phydev lookup IRQ-safe (RCU with
deferred free, or hold a device reference), or to document in the kernel-doc
that the caller must quiesce its link interrupt before
phylink_disconnect_phy()/phylink_destroy()?  The added kernel-doc currently
states no calling context or lifetime contract.

> +
>  static void phylink_resolve(struct work_struct *w)
>  {
>  	struct phylink *pl = container_of(w, struct phylink, resolve);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916-ax88179a-v12-0-60c04c9924a2%40birger-koblitz.de