Re: [PATCH net-next v20 01/13] phylink: Add phylink_mac_interrupt

[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 · 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] phylink_mac_interrupt() (drivers/net/phy/phylink.c:1629-1637)
  loads pl->phydev with no lock, no RCU and no reference, then calls…
- [Low] phylink_mac_interrupt() reads pl->phydev with a plain load (`phy
  = pl->phydev;`) and no lock.

--

phylink: Add phylink_mac_interrupt

This commit adds phylink_mac_interrupt(), an exported wrapper that passes a
link-change interrupt received by the MAC to phylib by calling
phy_mac_interrupt() on the PHY attached to the phylink instance. It is meant
for drivers that want to stop polling the PHY.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index e885da1c28311..a87eaa2f3db5b 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1616,6 +1616,26 @@ 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.
> + * This function may be called in interrupt context, and therefore cannot take
> + * a mutex. However, it is safe to assume that the reference to phy does not
> + * disappear during the call, as only an SFP module may be unplugged to make a
> + * PHY disappear. SFP modules however do not support interrupts.
> + */
> +void phylink_mac_interrupt(struct phylink *pl)
> +{
> +	struct phy_device *phy;
> +
> +	phy = pl->phydev;
> +	if (phy)
> +		phy_mac_interrupt(phy);

[Severity: Medium]
Is the lifetime argument in the kernel-doc above accurate? It says:

  "it is safe to assume that the reference to phy does not disappear
  during the call, as only an SFP module may be unplugged to make a PHY
  disappear. SFP modules however do not support interrupts."

phylink_disconnect_phy() is not specific to SFP. Any MAC driver can call it
under RTNL at runtime, for example from ndo_stop or on unbind.

Also, the interrupt here comes from the MAC, not the PHY. Whether an SFP PHY
supports interrupts doesn't seem to matter, and phylink_mac_interrupt() has
no check on phy->is_on_sfp_module or phy->irq.

phylink_disconnect_phy() clears pl->phydev under locks that this IRQ path
never takes. It then stops the PHY without waiting for lockless readers:

drivers/net/phy/phylink.c:phylink_disconnect_phy() {
    ...
		mutex_lock(&phy->lock);
		mutex_lock(&pl->state_mutex);
		pl->phydev = NULL;
    ...
	if (phy) {
		flush_work(&pl->resolve);
		phy_disconnect(phy);
	}
}

Could this interleaving happen?

CPU0 (MAC IRQ)                 CPU1
phylink_mac_interrupt()
  phy = pl->phydev;
                               phylink_disconnect_phy()
                                 pl->phydev = NULL;
                                 phy_disconnect(phy)
                                   phy_stop_machine()
                                     cancel_delayed_work_sync(
                                         &phydev->state_queue);
  phy_mac_interrupt(phy)
    phy_trigger_machine()
      phy_queue_state_machine()
        mod_delayed_work(..., &phydev->state_queue, ...);

If so, state_queue gets re-armed after the cancel has already finished.

On SFP module removal, sfp_sm_phy_detach() frees the phy_device right after
the disconnect:

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

sfp_remove_phy() reaches phylink_disconnect_phy() through
phylink_sfp_disconnect_phy(), and phy_device_remove() does not cancel
state_queue. Would this leave a delayed work queued inside a freed
phy_device? On the non-SFP paths, it would restart the state machine on a
stopped, detached PHY.

The only caller in this series, ax88179a_status(), does not seem to hit
this today. usbnet_stop()->usbnet_status_stop() kills the interrupt URB
before ax88179a_unbind() disconnects the PHY.

This is an exported, generic API, though. Should the kernel-doc tell
callers to quiesce their interrupt source before calling
phylink_disconnect_phy()? Or should phylink synchronize this itself, for
example with RCU and a grace period before phy_disconnect()?

[Severity: Low]
pl->phydev is read here with a plain load and no lock held.
phylink_bringup_phy() and phylink_disconnect_phy() write it with plain
stores under phydev_mutex and state_mutex. Will KCSAN report this as a data
race?

Without READ_ONCE(), the compiler may also reload pl->phydev after the NULL
check. The pointer passed to phy_mac_interrupt() could then differ from the
one that was tested. If a writer stored NULL in between, could
mod_delayed_work() end up called on &NULL->state_queue?

The current caller, ax88179a_status(), does not race with connect or
disconnect. Even so, would READ_ONCE() (or rcu_dereference()) here, paired
with WRITE_ONCE() (or rcu_assign_pointer()) in the writers, be appropriate?

> +}
> +EXPORT_SYMBOL_GPL(phylink_mac_interrupt);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-ax88179a-v20-0-37b5a1189983%40birger-koblitz.de