Re: [PATCH net v2 2/3] net: phy: remove phy ports upon probe failure

[Maxime Chevallier]

On 6/4/26 04:27, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: phy: remove phy ports upon probe failure
> 
> When phy_probe fails, let's clean the phy_ports that were successfully
> added already.
> 
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 6ccbfacf7d1d2..a171cbe2a74a1 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -3778,6 +3778,8 @@ static int phy_probe(struct device *dev)
>>  	return 0;
>>  
>>  out:
>> +	phy_cleanup_ports(phydev);
>> +
>>  	sfp_bus_del_upstream(phydev->sfp_bus);
>>  	phydev->sfp_bus = NULL;
>>  
> 
> [Severity: Critical]
> Is it safe to call phy_cleanup_ports() before sfp_bus_del_upstream() here?
> 
> sfp_bus_del_upstream() unregisters the upstream SFP callbacks and acts as a
> synchronization barrier by acquiring rtnl_lock(). Before it is called, the
> SFP bus is still active and can asynchronously fire events like
> phy_sfp_module_insert().
> 
> Because these SFP events locklessly iterate over the phydev->ports list via
> phy_get_sfp_port(), and phy_cleanup_ports() mutates the list and frees port
> structures without lock protection:
> 
> Could this concurrent access lead to list corruption and a use-after-free?
> 
> This same incorrect ordering is a pre-existing issue in the phy_remove()
> teardown path, but this patch introduces it into the phy_probe() error path.

Heh true indeed... Let me respin then, I'll fix the other callsite that has the
same problem

Thanks,

Maxime