Re: [RFC PATCH net-next 8/9] net: phylink: wait for PHYs that are known to probe late

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] pl->phydev is left as a dangling pointer if phylink_bringup_phy() fails, causing an infinite -EBUSY retry loop and a subsequent double-free/UAF.
--

commit 041169a410024c4510d91d095a3546cc79dd0bb6
Author: Aleksei Sviridkin <f@lex.la>

net: phylink: wait for PHYs that are known to probe late

A PHY whose driver or firmware lives on a filesystem mounted after the MAC
probes cannot be connected when the port is set up. This commit allows a port
to specify slow-to-probe, polling for the PHY instead of failing. It waits
for the PHY's driver to finish probing and re-tries on failures.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 3ec3bb4391095..62e6059a18da3 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -2254,6 +2277,129 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
>  }
>  EXPORT_SYMBOL_GPL(phylink_connect_phy);
>  
[ ... ]
> +static void phylink_slow_phy_poll(struct work_struct *work)
> +{
[ ... ]
> +	if (!ret) {
> +		ret = phylink_bringup_phy(pl, phy_dev,
> +					  pl->link_config.interface);
> +		if (ret)
> +			phy_detach(phy_dev);

[Severity: High]
This isn't a bug introduced by this patch, but the existing lack of rollback
in phylink_bringup_phy() is exacerbated by this new background poller, which
turns it into an infinite background loop and a guaranteed crash on teardown.

If phylink_bringup_phy() fails later in its execution (for example, if
phy_eee_rx_clock_stop() returns an error), it returns that error but leaves
pl->phydev pointing to the PHY:

drivers/net/phy/phylink.c:phylink_bringup_phy() {
    ...
    pl->phydev = phy;
    ...
    ret = phy_eee_rx_clock_stop(phy, pl->config->eee_rx_clk_stop_enable);
    if (ret == -EOPNOTSUPP)
        ret = 0;
    ...
    return ret;
}

Because pl->phydev remains populated after the failure, the poller calls
phy_detach() which drops the refcount. On the next poll cycle,
phylink_attach_phy() will unconditionally return -EBUSY because pl->phydev
is already set.

Furthermore, when the interface is later brought down and
phylink_disconnect_phy() is called, it finds pl->phydev set and calls
phy_disconnect(). This calls phy_detach() a second time on the
already-detached PHY, causing a double-put and use-after-free.

Should phylink_bringup_phy() clear pl->phydev on error, or should the caller
clear it if the bringup fails?

> +		else if (!test_bit(PHYLINK_DISABLE_STOPPED,
> +				   &pl->phylink_disable_state))
> +			/* phylink_start() starts the PHY it finds attached,
> +			 * and ran long before this one turned up.
> +			 */
> +			phy_start(phy_dev);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260829052546.1152446-1-f@lex.la?part=8