Re: [PATCH net-next v17 06/12] net: phylink: support late PCS provider attach

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: phylink: data race on link capabilities during FWNODE_PCS_PROVIDER_ADD
- [Medium] net: phylink: initialization data race via early PCS notifier registration
- [Medium] net: phylink: spurious link capability recalculation on duplicate PCS add
--

commit a7a4194143ec3bb732179891e559163a1b0eac1e
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: support late PCS provider attach

This commit introduces support for late PCS provider attachment in phylink.
It handles FWNODE_PCS_PROVIDER_ADD notifications by looking up and adding
the PCS to the phylink instance. If a PCS is added, it triggers interface
reconfiguration and recalculates link capabilities.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 019ce3d89da04..b87655a09df85 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -2006,27 +2027,57 @@ static int pcs_provider_notify(struct notifier_block *self,
>  	struct fwnode_pcs_provider *pp = data;
>  	struct phylink_pcs *pcs, *tmp;
>  	bool resolve = false;
> +	int count, i;
>  
> -	rtnl_lock();
> +	/*
> +	 * On PCS provider deletion hold rtnl lock as one of
> +	 * PCS can be currently in use by the phylink instance
> +	 * and ethtool OPs can reference it.
> +	 */
> +	if (val == FWNODE_PCS_PROVIDER_DEL)
> +		rtnl_lock();
>  
>  	mutex_lock(&pl->state_mutex);
>  
> -	/*
> -	 * Loop all the PCS for phylink instance and check if
> -	 * this notification is relevant for some of them.
> -	 */
> -	list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list) {
> -		if (!fwnode_pcs_matches_provider(pp, pl->fwnode, pcs))
> -			continue;
> +	switch (val) {
> +	case FWNODE_PCS_PROVIDER_ADD:

[Severity: Medium]
Can this new add event race with phylink_create() during initialization?

During phylink_create(), register_fwnode_pcs_notifier() is called to handle
hotplug events, prematurely exposing the instance before its initialization is
complete. Immediately after, phylink_create() mutates pl->supported and
pl->link_config.advertising locklessly.

If a concurrent FWNODE_PCS_PROVIDER_ADD event fires from another CPU,
this handler will execute and also mutate these same fields locklessly. Because
neither holds pl->state_mutex or rtnl_lock for these mutations, a data race
occurs.

> +		count = fwnode_phylink_pcs_count(pl->fwnode);
> +		for (i = 0; i < count; i++) {
> +			pcs = fwnode_pcs_get_from_provider(pp, pl->fwnode, i);
> +			if (IS_ERR(pcs))
> +				continue;
> +
> +			phylink_add_pcs(pl, pcs);
> +			resolve = true;

[Severity: Medium]
Will this unconditionally set resolve to true even if the PCS was already linked?

In phylink_add_pcs(), if the PCS is already present in pl->pcs_list, it silently
returns without adding it. Setting resolve = true regardless causes an unnecessary
recalculation of link capabilities and a spurious link resolve.

> +		}
> +
> +		/* Force an interface reconfig if major config fail */
> +		if (resolve && pl->major_config_failed)
> +			pl->force_major_config = true;
>  
> -		phylink_del_pcs(pl, pcs);
> -		resolve = true;
> +		break;
[ ... ]
> @@ -2046,7 +2097,8 @@ static int pcs_provider_notify(struct notifier_block *self,
>  	phylink_validate_mask(pl, NULL, pl->supported, &pl->link_config,
>  			      pl->supported_interfaces);
>  
> -	rtnl_unlock();
> +	if (val == FWNODE_PCS_PROVIDER_DEL)
> +		rtnl_unlock();

[Severity: High]
Does this code introduce a data race on the link capabilities during an add event?

For the FWNODE_PCS_PROVIDER_ADD event, rtnl_lock is not held. Before this code
executes, pl->state_mutex is also unlocked.

Then linkmode_fill() and phylink_validate_mask() destructively mutate
pl->supported and pl->link_config.advertising locklessly. This races with
concurrent ethtool operations (which expect rtnl_lock to protect capabilities)
and with other concurrent add notifications, potentially permanently corrupting
the bitmasks.

>  
>  	phylink_run_resolve(pl);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924230211.11519-1-ansuelsmth@gmail.com?part=6