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

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 [off-list ref]

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.

[ ... ]

[ ... ]

+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