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@xxxxxxxxxxx>
> ---
>  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