[PATCH net] net: phy: clean the sfp upstream if phy probing fails

[PATCH net] net: phy: clean the sfp upstream if phy probing fails

[Maxime Chevallier (Netdev Foundation)]

Sashiko reported that we don't call sfp_bus_del_upstream() in the probe
failure path, so let's add it, otherwise the sfp-bus is left with a
dangling 'upstream' field, that may be used later on during SFP events.

This issue existed before the generic phylib sfp support, back when
drivers were calling phy_sfp_probe themselves.

Fixes: 298e54fa810e ("net: phy: add core phylib sfp support")
Signed-off-by: Maxime Chevallier (Netdev Foundation) <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3370eb822017..6c2ef91fd197 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3775,6 +3775,9 @@ static int phy_probe(struct device *dev)
 	return 0;
 
 out:
+	sfp_bus_del_upstream(phydev->sfp_bus);
+	phydev->sfp_bus = NULL;
+
 	if (!phydev->is_on_sfp_module)
 		phy_led_triggers_unregister(phydev);
 
-- 
2.54.0

Re: [PATCH net] net: phy: clean the sfp upstream if phy probing fails

[Nicolai Buchwitz]

Hi Maxime

On 30.5.2026 09:27, Maxime Chevallier (Netdev Foundation) wrote:
> Sashiko reported that we don't call sfp_bus_del_upstream() in the probe
> failure path, so let's add it, otherwise the sfp-bus is left with a
> dangling 'upstream' field, that may be used later on during SFP events.
> 
> This issue existed before the generic phylib sfp support, back when
> drivers were calling phy_sfp_probe themselves.
> 
> Fixes: 298e54fa810e ("net: phy: add core phylib sfp support")
> Signed-off-by: Maxime Chevallier (Netdev Foundation) 
> <maxime.chevallier@bootlin.com>
> ---
>  drivers/net/phy/phy_device.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c 
> b/drivers/net/phy/phy_device.c
> index 3370eb822017..6c2ef91fd197 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3775,6 +3775,9 @@ static int phy_probe(struct device *dev)
>  	return 0;
> 
>  out:

While at it, should phy_cleanup_ports() also be added? Failure 
of_phy_leds() or
genphy_c45_read_eee_adv() would IMHO also produce a leak.

> +	sfp_bus_del_upstream(phydev->sfp_bus);
> +	phydev->sfp_bus = NULL;
> +
>  	if (!phydev->is_on_sfp_module)
>  		phy_led_triggers_unregister(phydev);

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net] net: phy: clean the sfp upstream if phy probing fails

[Maxime Chevallier]

On 5/30/26 14:33, Nicolai Buchwitz wrote:
> Hi Maxime
> 
> On 30.5.2026 09:27, Maxime Chevallier (Netdev Foundation) wrote:
>> Sashiko reported that we don't call sfp_bus_del_upstream() in the probe
>> failure path, so let's add it, otherwise the sfp-bus is left with a
>> dangling 'upstream' field, that may be used later on during SFP events.
>>
>> This issue existed before the generic phylib sfp support, back when
>> drivers were calling phy_sfp_probe themselves.
>>
>> Fixes: 298e54fa810e ("net: phy: add core phylib sfp support")
>> Signed-off-by: Maxime Chevallier (Netdev Foundation) <maxime.chevallier@bootlin.com>
>> ---
>>  drivers/net/phy/phy_device.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 3370eb822017..6c2ef91fd197 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -3775,6 +3775,9 @@ static int phy_probe(struct device *dev)
>>      return 0;
>>
>>  out:
> 
> While at it, should phy_cleanup_ports() also be added? Failure of_phy_leds() or
> genphy_c45_read_eee_adv() would IMHO also produce a leak.

Good point, I'll followup with this :) Thank you !

Maxime

Re: [PATCH net] net: phy: clean the sfp upstream if phy probing fails

[Maxime Chevallier]

Hi,

On 5/30/26 09:27, Maxime Chevallier (Netdev Foundation) wrote:
> Sashiko reported that we don't call sfp_bus_del_upstream() in the probe
> failure path, so let's add it, otherwise the sfp-bus is left with a
> dangling 'upstream' field, that may be used later on during SFP events.
> 
> This issue existed before the generic phylib sfp support, back when
> drivers were calling phy_sfp_probe themselves.
> 
> Fixes: 298e54fa810e ("net: phy: add core phylib sfp support")
> Signed-off-by: Maxime Chevallier (Netdev Foundation) <maxime.chevallier@bootlin.com>
> ---
>  drivers/net/phy/phy_device.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 3370eb822017..6c2ef91fd197 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3775,6 +3775,9 @@ static int phy_probe(struct device *dev)
>  	return 0;
>  
>  out:
> +	sfp_bus_del_upstream(phydev->sfp_bus);
> +	phydev->sfp_bus = NULL;
> +
>  	if (!phydev->is_on_sfp_module)
>  		phy_led_triggers_unregister(phydev);
>  
Sashiko said :

"
If sfp_bus_add_upstream() fails during phy_sfp_probe(), will this cause a
use-after-free and double free?

When sfp_bus_add_upstream() fails, it drops its own internal reference, and
phy_sfp_probe() calls sfp_bus_put(), which frees the bus.

However, phydev->sfp_bus was previously assigned to bus and is not cleared.
If phy_probe() then jumps to the out: label, calling sfp_bus_del_upstream()
will access the already freed memory and unconditionally call sfp_bus_put()
again.
"

It also pointed out the usual set of pre-existing issues...

So with this and Nicolai's comment, I'll sent a new iteration as a series with
some more cleanups.

Nicolai, I'll likely modify this current patch to address sashiko's comment, I'll
have to drop your review tag unfortunately :(

Maxime