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

[Jakub Kicinski]

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.
-- 
pw-bot: cr