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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] phylink: Use-After-Free in phylink_mac_interrupt() on SFP unplug
--

commit 30d7b430d60b3d4dc2e6d650aaf0ceaacfbadbce
Author: Birger Koblitz <mail@birger-koblitz.de>

phylink: Add phylink_mac_interrupt

This commit adds the phylink_mac_interrupt() function to phylink, allowing
MAC drivers to pass link-change interrupts directly to phylib instead of
relying on polling.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a7d086cdc9b25..a3ca7993eb7e0 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -1612,6 +1612,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: High]
Can this sequence lead to a use-after-free if an SFP module is unplugged
concurrently?

The docstring mentions that SFP modules do not support interrupts, but if a
MAC driver detects a Loss of Signal and calls phylink_mac_interrupt(),
couldn't the SFP subsystem concurrently detect the unplug and trigger
phylink_disconnect_phy()?

If that happens, phylink_disconnect_phy() takes phydev_mutex, clears
pl->phydev, and calls phy_disconnect() which drops the object reference and
frees the phy_device.

Since pl->phydev is read locklessly here, is it possible for
phylink_mac_interrupt() to read a non-NULL pointer, but hand it to
phy_mac_interrupt() after it has been freed?

CPU1 (SFP unplug)                 CPU2 (MAC Interrupt)
phylink_disconnect_phy()
  mutex_lock(&pl->phydev_mutex)
  phy = pl->phydev
  pl->phydev = NULL
  phy_disconnect(phy)
                                  phy = pl->phydev; (Reads old pointer)
    phy_detach()
      phy_device_free()
                                  if (phy)
                                    phy_mac_interrupt(phy)
                                      phy_trigger_machine(phy)
                                        mod_delayed_work(...)

This could then cause list corruption when phy_trigger_machine() queues work
on the freed phydev->state_queue.

> +}
> +EXPORT_SYMBOL_GPL(phylink_mac_interrupt);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-ax88179a-v17-0-252814fe35ea@birger-koblitz.de?part=1