[PATCH net v5 0/6] net: phy: fix cleanup after probe failure

[PATCH net v5 0/6] net: phy: fix cleanup after probe failure

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

phy_probe() initializes the PHY driver, ports, SFP upstream, and LEDs in
stages. Its error paths do not always release only the resources acquired
at each stage. It can also mark the PHY ready before all setup succeeds.

Port setup also leaves SFP cleanup split between phy_sfp_probe(),
phy_setup_ports(), and phy_probe(), and default port setup ignores errors
from attaching the port to the PHY driver.

This series makes each initialization layer own its cleanup and propagates
setup failures to the caller.

Patch 1 splits the phy_probe() cleanup by initialization stage.

Patch 2 makes SFP and port setup unwind their resources in the required
order.

Patch 3 sets PHY_READY only after LED setup succeeds.

Patch 4 calls the PHY driver remove callback after later probe failures.

Patch 5 propagates errors from default port setup.

Patch 6 prevents a double-free after partial LED trigger registration.

---
Changes:
v5:
  Patch 1:
  - Describe the NULL-dereference path after a probe/remove cycle and add
    the corresponding Fixes tag. (Sashiko.)

  Patch 5:
  - Correct the Reviewed-by tag to Maxime Chevallier.

  Patch 6:
  - Mention the phy_remove() teardown path that can also trigger the
    double-free. (Sashiko.)

v4: https://lore.kernel.org/all/20260821070327.16147-1-xuanqiang.luo@linux.dev/
  Patch 1:
  - Explain that LED triggers are initialized after the SFP upstream and
    ports, so they must be unwound first. (Andrew Lunn.)

  Patch 2-5:
  - Add Reviewed-by: Andrew Lunn <andrew@lunn.ch>.

  Patch 6 (new):
  - Prevent a double-free after partial LED trigger registration and make
    the cleanup ownership explicit. (Sashiko.)

v3: https://lore.kernel.org/all/20260819060236.24665-1-xuanqiang.luo@linux.dev/
  Patch 1:
  - Rename cleanup labels to include verbs describing their actions.
    (Jakub Kicinski.)

  Patch 3:
  - Do not clear phydev->drv before device-core teardown completes; this
    can expose NULL dereferences in concurrent attach paths and devres
    callbacks. Set PHY_READY only after LED setup succeeds and update the
    Fixes tag. (Sashiko, Jakub Kicinski.)

v2: https://lore.kernel.org/all/20260813132946.116176-1-xuanqiang.luo@linux.dev/
  Patch 1:
  - Limit this patch to splitting phy_probe() error paths, moving the SFP
    teardown fixes to Patch 2.

  Patch 2 (new):
  - Make SFP and port setup unwind their resources in the required order.
  - Add phy_sfp_release() for complete SFP teardown instead of open-coding
    sfp_bus_del_upstream(). (Andrew Lunn, Maxime Chevallier.)

  Patch 3 (new):
  - Restore PHY_DOWN and clear phydev->drv after probe failure.

  Patch 4:
  - Move the former Patch 2 to Patch 4; no functional changes.

  Patch 5 (new):
  - Propagate errors from default port setup.

v1: https://lore.kernel.org/all/20260812125127.106255-1-xuanqiang.luo@linux.dev/

Xuanqiang Luo (6):
  net: phy: split phy_probe() error paths
  net: phy: unregister SFP upstream before port cleanup
  net: phy: set PHY_READY after LED setup
  net: phy: call driver remove when core initialization fails
  net: phy: propagate errors from default port setup
  net: phy: avoid double-free after LED trigger registration failure

 drivers/net/phy/phy_device.c       | 80 ++++++++++++++++++++++--------
 drivers/net/phy/phy_led_triggers.c | 11 ++--
 2 files changed, 65 insertions(+), 26 deletions(-)


base-commit: 746fc0787f616da418ffc04a110296fe95d53491
-- 
2.43.0

[PATCH net v5 1/6] net: phy: split phy_probe() error paths

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

phy_probe() uses one cleanup path for failures at every initialization
stage. This runs cleanup for resources that have not been initialized.

After a successful probe and remove, phy_led_triggers_unregister() can
leave phy_num_led_triggers non-zero after freeing the trigger array. If a
subsequent probe fails before LED trigger registration, the common error
path calls phy_led_triggers_unregister() with a NULL array and stale count,
causing a NULL dereference.

Split the cleanup by initialization stage so each failure path unwinds only
the resources that may have been initialized. Unregister LED triggers
before releasing the SFP upstream and ports, because the LED triggers are
initialized after those resources and must be unwound first.

Fixes: c8dbdc6e380e ("net: phy: register phy led_triggers during probe to avoid AB-BA deadlock")
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/phy_device.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 94b2e85e00a37..2cf70471ae089 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3706,7 +3706,7 @@ static int phy_probe(struct device *dev)
 	if (phydev->drv->probe) {
 		err = phydev->drv->probe(phydev);
 		if (err)
-			goto out;
+			goto out_reset;
 	}
 
 	phy_disable_interrupts(phydev);
@@ -3727,7 +3727,7 @@ static int phy_probe(struct device *dev)
 		err = genphy_read_abilities(phydev);
 
 	if (err)
-		goto out;
+		goto out_reset;
 
 	if (!linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
 			       phydev->supported))
@@ -3744,7 +3744,7 @@ static int phy_probe(struct device *dev)
 
 	err = phy_setup_ports(phydev);
 	if (err)
-		goto out;
+		goto out_sfp_release;
 
 	phy_advertise_supported(phydev);
 
@@ -3753,7 +3753,7 @@ static int phy_probe(struct device *dev)
 	 */
 	err = genphy_c45_read_eee_adv(phydev, phydev->advertising_eee);
 	if (err)
-		goto out;
+		goto out_sfp_release;
 
 	/* Get the EEE modes we want to prohibit. */
 	of_set_phy_eee_broken(phydev);
@@ -3806,20 +3806,22 @@ static int phy_probe(struct device *dev)
 	if (IS_ENABLED(CONFIG_PHYLIB_LEDS) && !phy_driver_is_genphy(phydev)) {
 		err = of_phy_leds(phydev);
 		if (err)
-			goto out;
+			goto out_unreg_led_triggers;
 	}
 
 	return 0;
 
-out:
+out_unreg_led_triggers:
+	if (!phydev->is_on_sfp_module)
+		phy_led_triggers_unregister(phydev);
+
+out_sfp_release:
 	sfp_bus_del_upstream(phydev->sfp_bus);
 	phydev->sfp_bus = NULL;
 
 	phy_cleanup_ports(phydev);
 
-	if (!phydev->is_on_sfp_module)
-		phy_led_triggers_unregister(phydev);
-
+out_reset:
 	/* Re-assert the reset signal on error */
 	phy_device_reset(phydev, 1);
 
-- 
2.43.0

[PATCH net v5 2/6] net: phy: unregister SFP upstream before port cleanup

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

Commit 4497f5028675 ("net: phy: Clean the phy_ports after unregistering
the downstream SFP bus") established that an SFP upstream must be
unregistered before its phy_ports are destroyed because SFP callbacks
may access these ports.

phy_setup_ports() does not follow this order when a later port setup
step fails after phy_sfp_probe() succeeds. It destroys the SFP phy_port
and leaves phy_probe() to unregister the upstream later, creating a race
between port destruction and SFP upstream callbacks.

The error unwind is also split across three functions. If
phy_setup_sfp_port() fails, phy_sfp_probe() leaves the upstream
registered and relies on phy_probe() to remove it after
phy_setup_ports() returns.

Make each layer unwind the resources it successfully set up. Unregister
only the upstream in phy_sfp_probe() when SFP port setup fails, since
the failed port has already been destroyed. Add phy_sfp_release() for a
successful SFP probe, and make phy_setup_ports() use it before cleaning
up the remaining ports. Once phy_setup_ports() has rolled back all port
setup, make phy_probe() skip this cleanup.

Fixes: 589e934d2735 ("net: phy: Introduce PHY ports representation")
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/phy_device.c | 49 ++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2cf70471ae089..4b9b2300422fb 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1723,12 +1723,41 @@ static int phy_sfp_probe(struct phy_device *phydev)
 			phydev->sfp_bus = NULL;
 	}
 
-	if (!ret && phydev->sfp_bus)
+	if (!ret && phydev->sfp_bus) {
 		ret = phy_setup_sfp_port(phydev);
+		if (ret) {
+			sfp_bus_del_upstream(phydev->sfp_bus);
+			phydev->sfp_bus = NULL;
+		}
+	}
 
 	return ret;
 }
 
+/**
+ * phy_sfp_release - release resources set up by phy_sfp_probe()
+ * @phydev: the PHY device
+ *
+ * Release the SFP resources set up by a successful phy_sfp_probe(). Unregister
+ * the upstream before destroying its phy_port, so SFP upstream callbacks cannot
+ * race with port destruction.
+ */
+static void phy_sfp_release(struct phy_device *phydev)
+{
+	struct phy_port *port, *tmp;
+
+	sfp_bus_del_upstream(phydev->sfp_bus);
+	phydev->sfp_bus = NULL;
+
+	list_for_each_entry_safe(port, tmp, &phydev->ports, head) {
+		if (!port->is_sfp)
+			continue;
+
+		phy_del_port(phydev, port);
+		phy_port_destroy(port);
+	}
+}
+
 static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
 {
 	return phydrv->config_intr && phydrv->handle_interrupt;
@@ -3547,13 +3576,13 @@ static int phy_setup_ports(struct phy_device *phydev)
 	if (!phydev->is_genphy_driven) {
 		ret = phy_sfp_probe(phydev);
 		if (ret)
-			goto out;
+			goto err_ports;
 	}
 
 	if (phydev->n_ports < phydev->max_n_ports) {
 		ret = phy_default_setup_single_port(phydev);
 		if (ret)
-			goto out;
+			goto err_sfp;
 	}
 
 	linkmode_zero(ports_supported);
@@ -3580,7 +3609,9 @@ static int phy_setup_ports(struct phy_device *phydev)
 
 	return 0;
 
-out:
+err_sfp:
+	phy_sfp_release(phydev);
+err_ports:
 	phy_cleanup_ports(phydev);
 	return ret;
 }
@@ -3744,7 +3775,7 @@ static int phy_probe(struct device *dev)
 
 	err = phy_setup_ports(phydev);
 	if (err)
-		goto out_sfp_release;
+		goto out_reset;
 
 	phy_advertise_supported(phydev);
 
@@ -3816,9 +3847,7 @@ static int phy_probe(struct device *dev)
 		phy_led_triggers_unregister(phydev);
 
 out_sfp_release:
-	sfp_bus_del_upstream(phydev->sfp_bus);
-	phydev->sfp_bus = NULL;
-
+	phy_sfp_release(phydev);
 	phy_cleanup_ports(phydev);
 
 out_reset:
@@ -3842,9 +3871,7 @@ static int phy_remove(struct device *dev)
 
 	phydev->state = PHY_DOWN;
 
-	sfp_bus_del_upstream(phydev->sfp_bus);
-	phydev->sfp_bus = NULL;
-
+	phy_sfp_release(phydev);
 	phy_cleanup_ports(phydev);
 
 	if (phydev->drv && phydev->drv->remove)
-- 
2.43.0

[PATCH net v5 3/6] net: phy: set PHY_READY after LED setup

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

phy_probe() sets PHY_READY before calling of_phy_leds(). If LED setup
fails, the error path releases the initialized resources while the PHY
state remains READY even though probing failed.

Set PHY_READY only after LED setup succeeds.

Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/phy_device.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 4b9b2300422fb..891df46d0597f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3824,9 +3824,6 @@ static int phy_probe(struct device *dev)
 				 phydev->supported);
 	}
 
-	/* Set the state to READY by default */
-	phydev->state = PHY_READY;
-
 	/* Register the PHY LED triggers */
 	if (!phydev->is_on_sfp_module)
 		phy_led_triggers_register(phydev);
@@ -3840,6 +3837,9 @@ static int phy_probe(struct device *dev)
 			goto out_unreg_led_triggers;
 	}
 
+	/* Set the state to READY by default */
+	phydev->state = PHY_READY;
+
 	return 0;
 
 out_unreg_led_triggers:
-- 
2.43.0

[PATCH net v5 4/6] net: phy: call driver remove when core initialization fails

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

phy_probe() may fail while querying features or completing other core
initialization after the PHY driver probe callback has succeeded. The
driver core does not run the remove path after a probe error, so
resources that the PHY driver releases in its remove callback are
leaked.

Call the PHY driver remove callback on these failures.

Fixes: efbdfdc29bdd ("net: phy: Add support for asking the PHY its abilities")
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/phy_device.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 891df46d0597f..691396794decd 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3758,7 +3758,7 @@ static int phy_probe(struct device *dev)
 		err = genphy_read_abilities(phydev);
 
 	if (err)
-		goto out_reset;
+		goto out_remove;
 
 	if (!linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
 			       phydev->supported))
@@ -3775,7 +3775,7 @@ static int phy_probe(struct device *dev)
 
 	err = phy_setup_ports(phydev);
 	if (err)
-		goto out_reset;
+		goto out_remove;
 
 	phy_advertise_supported(phydev);
 
@@ -3850,6 +3850,10 @@ static int phy_probe(struct device *dev)
 	phy_sfp_release(phydev);
 	phy_cleanup_ports(phydev);
 
+out_remove:
+	if (phydev->drv->remove)
+		phydev->drv->remove(phydev);
+
 out_reset:
 	/* Re-assert the reset signal on error */
 	phy_device_reset(phydev, 1);
-- 
2.43.0

[PATCH net v5 5/6] net: phy: propagate errors from default port setup

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

phy_default_setup_single_port() ignores errors from phy_add_port() and
always reports success. If a PHY driver attach_mdi_port() callback fails,
the phy_port is leaked and PHY probing continues without the expected
default port.

Destroy the port and return the error.

Fixes: 589e934d2735 ("net: phy: Introduce PHY ports representation")
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/phy_device.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 691396794decd..8cb0d60fcbba9 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3483,6 +3483,7 @@ static int phy_default_setup_single_port(struct phy_device *phydev)
 {
 	struct phy_port *port = phy_port_alloc();
 	unsigned long mode;
+	int ret;
 
 	if (!port)
 		return -ENOMEM;
@@ -3509,9 +3510,11 @@ static int phy_default_setup_single_port(struct phy_device *phydev)
 		port->pairs = max_t(int, port->pairs,
 				    ethtool_linkmode_n_pairs(mode));
 
-	phy_add_port(phydev, port);
+	ret = phy_add_port(phydev, port);
+	if (ret)
+		phy_port_destroy(port);
 
-	return 0;
+	return ret;
 }
 
 static int of_phy_ports(struct phy_device *phydev)
-- 
2.43.0

[PATCH net v5 6/6] net: phy: avoid double-free after LED trigger registration failure

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

phy_led_triggers_register() frees phy_led_triggers when a speed trigger
registration fails, but leaves the pointer set to the freed allocation
before clearing phy_num_led_triggers.

phy_probe() ignores the registration error. If a later probe step fails,
its error path calls phy_led_triggers_unregister(); normal teardown during
an unbind or MDIO bus removal calls the same helper from phy_remove().
In either case, the trigger count is zero, so the per-trigger unregister
loop is skipped, but the dangling pointer is still freed unconditionally.

Clear the pointer when partial registration cleanup frees the array, and
make phy_led_triggers_unregister() free the array only when its pointer is
non-NULL.

Fixes: b7f0ee992adf ("net: phy: leds: fix memory leak")
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/phy_led_triggers.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy_led_triggers.c b/drivers/net/phy/phy_led_triggers.c
index 4eb7716bb9d6c..ff6e518395be0 100644
--- a/drivers/net/phy/phy_led_triggers.c
+++ b/drivers/net/phy/phy_led_triggers.c
@@ -126,6 +126,7 @@ int phy_led_triggers_register(struct phy_device *phy)
 	while (i--)
 		phy_led_trigger_unregister(&phy->phy_led_triggers[i]);
 	kfree(phy->phy_led_triggers);
+	phy->phy_led_triggers = NULL;
 out_unreg_link:
 	phy_led_trigger_unregister(phy->led_link_trigger);
 out_free_link:
@@ -141,10 +142,12 @@ void phy_led_triggers_unregister(struct phy_device *phy)
 {
 	int i;
 
-	for (i = 0; i < phy->phy_num_led_triggers; i++)
-		phy_led_trigger_unregister(&phy->phy_led_triggers[i]);
-	kfree(phy->phy_led_triggers);
-	phy->phy_led_triggers = NULL;
+	if (phy->phy_led_triggers) {
+		for (i = 0; i < phy->phy_num_led_triggers; i++)
+			phy_led_trigger_unregister(&phy->phy_led_triggers[i]);
+		kfree(phy->phy_led_triggers);
+		phy->phy_led_triggers = NULL;
+	}
 
 	if (phy->led_link_trigger) {
 		phy_led_trigger_unregister(phy->led_link_trigger);
-- 
2.43.0