From b6d201d9714531df2b0194ec7f4a4bcee0a89238 Mon Sep 17 00:00:00 2001 From: Xuanqiang Luo Date: Sun, 23 Aug 2026 11:55:55 +0800 Subject: [PATCH 1/6] net: phy: split phy_probe() error paths 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. Signed-off-by: Xuanqiang Luo Link: https://patch.msgid.link/20260823035600.188864-2-xuanqiang.luo@linux.dev Signed-off-by: Jakub Kicinski --- 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 94b2e85e00a3..2cf70471ae08 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); From f558204abcb905adc492f74a9536c794b9371a23 Mon Sep 17 00:00:00 2001 From: Xuanqiang Luo Date: Sun, 23 Aug 2026 11:55:56 +0800 Subject: [PATCH 2/6] net: phy: unregister SFP upstream before port cleanup 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. Reviewed-by: Andrew Lunn Signed-off-by: Xuanqiang Luo Link: https://patch.msgid.link/20260823035600.188864-3-xuanqiang.luo@linux.dev Signed-off-by: Jakub Kicinski --- 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 2cf70471ae08..4b9b2300422f 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) From 52dd7eb1a33e51f781e882cfed01de6412613fbc Mon Sep 17 00:00:00 2001 From: Xuanqiang Luo Date: Sun, 23 Aug 2026 11:55:57 +0800 Subject: [PATCH 3/6] net: phy: set PHY_READY after LED setup 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. Reviewed-by: Andrew Lunn Signed-off-by: Xuanqiang Luo Link: https://patch.msgid.link/20260823035600.188864-4-xuanqiang.luo@linux.dev Signed-off-by: Jakub Kicinski --- 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 4b9b2300422f..891df46d0597 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: From afd5aecd3c0721a04609992a37904b925e2097f6 Mon Sep 17 00:00:00 2001 From: Xuanqiang Luo Date: Sun, 23 Aug 2026 11:55:58 +0800 Subject: [PATCH 4/6] net: phy: call driver remove when core initialization fails 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. Reviewed-by: Andrew Lunn Signed-off-by: Xuanqiang Luo Link: https://patch.msgid.link/20260823035600.188864-5-xuanqiang.luo@linux.dev Signed-off-by: Jakub Kicinski --- 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 891df46d0597..691396794dec 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); From 48378efebbe7bf7a2533bc59a7acb94fcf46021e Mon Sep 17 00:00:00 2001 From: Xuanqiang Luo Date: Sun, 23 Aug 2026 11:55:59 +0800 Subject: [PATCH 5/6] net: phy: propagate errors from default port setup 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. Reviewed-by: Maxime Chevallier Signed-off-by: Xuanqiang Luo Link: https://patch.msgid.link/20260823035600.188864-6-xuanqiang.luo@linux.dev Signed-off-by: Jakub Kicinski --- 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 691396794dec..8cb0d60fcbba 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) From f7f619ecda5761bd48578764ef5ff9e8fd97b916 Mon Sep 17 00:00:00 2001 From: Xuanqiang Luo Date: Sun, 23 Aug 2026 11:56:00 +0800 Subject: [PATCH 6/6] net: phy: avoid double-free after LED trigger registration failure 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. Signed-off-by: Xuanqiang Luo Link: https://patch.msgid.link/20260823035600.188864-7-xuanqiang.luo@linux.dev Signed-off-by: Jakub Kicinski --- 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 4eb7716bb9d6..ff6e518395be 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);