Re: [PATCH v14 06/12] net: phylink: support late PCS provider attach

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing `rtnl_lock()` during late PCS provider attachment causes a data race on `pl->supported_interfaces`.
- [High] Late-attached PCS capabilities are not propagated to ethtool supported linkmodes (`pl->supported`).
- [High] If the PCS provider's `fwnode_xlate` callback returns `NULL`, it leads to a kernel panic in `phylink_add_pcs()`.
--

--- Patch [6]: [PATCH v14 06/12] net: phylink: support late PCS provider attach ---
commit b6137c0cfab8a7a646c6cbf7cf50ad62dca0f51b
Author: Christian Marangi <[email protected]>

net: phylink: support late PCS provider attach
    
Add support for late PCS provider attachment to a phylink instance.
Similar to FWNODE_PCS_PROVIDER_DEL, FWNODE_PCS_PROVIDER_ADD is added to
address the case where a PCS provider is added after a phylink instance is
created and started.

> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> index 25b3248e8b93b..fef560eddf2a9 100644
> --- a/drivers/net/pcs/pcs.c
> +++ b/drivers/net/pcs/pcs.c
[ ... ]
> @@ -201,6 +205,24 @@ struct phylink_pcs *fwnode_pcs_get(const struct fwnode_handle *fwnode, unsigned
>  }
>  EXPORT_SYMBOL_GPL(fwnode_pcs_get);
>  
> +struct phylink_pcs *fwnode_pcs_get_from_provider(struct fwnode_pcs_provider *provider,
> +						 const struct fwnode_handle *fwnode,
> +						 int index)
> +{
> +	struct fwnode_reference_args pcsspec;
> +	struct phylink_pcs *pcs;
> +	int ret;
> +
> +	ret = fwnode_parse_pcsspec(fwnode, index, NULL, &pcsspec);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	pcs = __fwnode_pcs_get_from_pcsspec_provider(&pcsspec, provider);
> +	fwnode_handle_put(pcsspec.fwnode);
> +	return pcs;

[Severity: High]
If the provider's fwnode_xlate callback returns NULL, this function will
return NULL.

Unlike fwnode_pcs_get_from_pcsspec(), which sanitizes NULL to
ERR_PTR(-ENODEV), this function returns NULL directly.

Could this return value lead to a NULL pointer dereference in
pcs_provider_notify() where IS_ERR() is checked instead of IS_ERR_OR_NULL()?

> +}
> +EXPORT_SYMBOL_GPL(fwnode_pcs_get_from_provider);
> +
[ ... ]
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 967cc919a3656..ba3efdb0a1948 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -2004,27 +2025,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:
> +		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);

[Severity: High]
If pcs is NULL here (because fwnode_pcs_get_from_provider() returned NULL),
the IS_ERR() check will pass. 

Would this cause a kernel panic when phylink_add_pcs() dereferences pcs
to add it to the list via list_add_tail(&pcs->list, &pl->pcs_list)?

> +			resolve = true;
> +		}
> +
> +		/* 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;
[ ... ]
> @@ -2038,7 +2089,8 @@ static int pcs_provider_notify(struct notifier_block *self,
>  
>  	mutex_unlock(&pl->state_mutex);
>  

[Severity: High]
Is it safe to update pl->supported_interfaces without holding rtnl_lock()
during FWNODE_PCS_PROVIDER_ADD?

The FWNODE_PCS_PROVIDER_ADD path intentionally omits taking rtnl_lock() and
modifies pl->supported_interfaces non-atomically via phy_interface_or()
and phy_interface_copy() just above this.

Concurrently, phylink_sfp_config_optical() reads pl->supported_interfaces
under rtnl_lock() but outside pl->state_mutex. Could this lead to a torn
read of the bitmap, causing valid link modes to be spuriously rejected and
SFP links failing to come up?

[Severity: High]
Does pl->supported (the ethtool linkmode mask) need to be recalculated when
new PCS capabilities are added?

This code refreshes pl->supported_interfaces, but it doesn't appear to
recalculate the ethtool capabilities mask pl->supported, which is normally
done in phylink_create() via linkmode_fill(), linkmode_copy(), and
phylink_validate(). 

If pl->supported is not updated here, will it remain permanently stale and
prevent the use of new speeds or link modes introduced by the late-attached
PCS?

> -	rtnl_unlock();
> +	if (val == FWNODE_PCS_PROVIDER_DEL)
> +		rtnl_unlock();
>  
>  	phylink_run_resolve(pl);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6