Re: [PATCH net-next v18 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 configuration bitmaps during phylink_create()
- [High] net: phylink: missing ownership check in phylink_add_pcs() allows list corruption
- [High] net: phylink: missing NULL check on fwnode_pcs_get_from_provider() causes panic
--

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

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/phy/phylink.c b/drivers/net/phy/phylink.c
> index 019ce3d89da04..59283564f72ed 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1928,6 +1928,30 @@ int phylink_set_fixed_link(struct phylink *pl,
>  }
>  EXPORT_SYMBOL_GPL(phylink_set_fixed_link);
>  
> +static bool phylink_add_pcs(struct phylink *pl, struct phylink_pcs *pcs)
> +{
> +	struct phylink_pcs *tmp;
> +
> +	/*
> +	 * Make sure state mutex is locked to protect concurrent
> +	 * access to phylink instance PCS list from
> +	 * initial fill_available_pcs and late PCS attach
> +	 */
> +	lockdep_assert_held(&pl->state_mutex);
> +
> +	/* Make sure PCS is not already in the list */
> +	list_for_each_entry(tmp, &pl->pcs_list, list)
> +		if (tmp == pcs)
> +			return false;
> +
> +	list_add_tail(&pcs->list, &pl->pcs_list);
> +
> +	/* Link PCS to phylink */
> +	pcs->phylink = pl;

[Severity: High]
Does this code allow corrupting another phylink instance's list?

If a firmware node misconfigures multiple MACs to reference the same PCS,
or a provider returns a singleton without exclusivity checks, the same
phylink_pcs could be returned to multiple phylink instances.

This checks if the PCS is already in the current instance's list, but
doesn't check if it is already owned by another instance (for example,
by checking if pcs->phylink && pcs->phylink != pl).

If list_add_tail() is called when the PCS is already linked elsewhere,
could this silently overwrite its list pointers and corrupt the first
instance's list?

> +
> +	return true;
> +}
> +

[ ... ]

> @@ -2006,27 +2030,51 @@ 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();
>  
>  	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: High]
Does this create a data race between phylink_create() and pcs_provider_notify()?

In phylink_create(), the pl instance is exposed to the notifier chain before
the bitmaps are initialized:

phylink_create() {
  ...
  register_fwnode_pcs_notifier(&pl->fwnode_pcs_nb);
  ...
  linkmode_fill(pl->supported);
  linkmode_copy(pl->link_config.advertising, pl->supported);
}

Because phylink_create() does not hold rtnl_lock(), if a new PCS provider is
added concurrently, this notification could fire on another CPU, acquire
rtnl_lock(), and modify these exact same bitmaps simultaneously. Could this
corrupt the advertised interfaces or capabilities?

> +		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;

[Severity: High]
Is it possible for fwnode_pcs_get_from_provider() to return NULL here?

Subsystem translation callbacks (like the provider's fwnode_xlate)
commonly return NULL to indicate no device is found. However, IS_ERR(NULL)
is false, which means a NULL pointer would bypass this check.

If pcs is NULL, it will be passed to phylink_add_pcs() where
list_add_tail(&pcs->list, &pl->pcs_list) and pcs->phylink = pl
will dereference it. Could this cause a kernel panic?

> +
> +			/* Trigger a resolve only if a PCS is actually added */
> +			if (phylink_add_pcs(pl, pcs))
> +				resolve = true;
> +		}
> +

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