Re: [RFC PATCH net-next v8 05/12] net: phylink: support late PCS provider attach

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Unconditional `list_add()` in `pcs_provider_notify()` leads to list corruption for shared PCS providers.
- [Critical] Missing PCS removal handling leads to Use-After-Free in `pl->pcs_list`.
- [High] TOCTOU race in `phylink_create()` allows a late PCS provider to be permanently missed.
- [High] Incorrect fwnode used for `pcs-handle` lookup silently breaks late attach on multi-port devices (e.g. DSA).
- [High] Late attach updates supported interfaces but leaves ethtool linkmodes stale.
- [High] `pcs_provider_notify()` ignores subsequent PCS instances if a MAC requires multiple PCSs from the same provider fwnode.
- [High] Self-deadlock due to unconditional `rtnl_lock()` acquisition in the PCS notifier callback.
--

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

net: phylink: support late PCS provider attach

This commit adds support for late PCS provider attachment in phylink.
It creates a global notifier chain for PCS providers and allows phylink
instances to subscribe and dynamically add PCS providers when they become
available.

> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> index 0cc4daf7beeae..7a9d91b2a34da 100644
> --- a/drivers/net/pcs/pcs.c
> +++ b/drivers/net/pcs/pcs.c
[ ... ]
> @@ -150,6 +167,38 @@ struct phylink_pcs *fwnode_pcs_get(struct fwnode_handle *fwnode, unsigned int in
>  }
>  EXPORT_SYMBOL_GPL(fwnode_pcs_get);
>  
> +struct phylink_pcs *
> +fwnode_phylink_pcs_get_from_fwnode(struct fwnode_handle *fwnode,
> +				   struct fwnode_handle *pcs_fwnode)
> +{
> +	struct fwnode_reference_args pcsspec;
> +	int index = 0;
> +	int ret;
> +
> +	/* Loop until we find a matching PCS node or
> +	 * fwnode_parse_pcsspec() returns error
> +	 * if we don't have any other PCS reference to check.
> +	 */
> +	while (true) {
> +		ret = fwnode_parse_pcsspec(fwnode, index, NULL, &pcsspec);
> +		if (ret)
> +			return ERR_PTR(ret);
> +
> +		/* Exit loop if we found the matching PCS node */
> +		if (pcsspec.fwnode == pcs_fwnode) {
> +			fwnode_handle_put(pcsspec.fwnode);
> +			break;
> +		}

[Severity: High]
Does breaking out of the loop on the first match ignore subsequent PCS 
instances if a MAC requires multiple PCSs from the same provider?

If a phylink instance is configured to use multiple PCSs from the same 
provider, it seems the subsequent indices will never be discovered since the 
notifier only fires once per provider fwnode.

> +
> +		/* Check the next PCS reference */
> +		fwnode_handle_put(pcsspec.fwnode);
> +		index++;
> +	}
> +
> +	return fwnode_pcs_get(fwnode, index);
> +}
> +EXPORT_SYMBOL_GPL(fwnode_phylink_pcs_get_from_fwnode);
> +
>  unsigned int fwnode_phylink_pcs_count(struct fwnode_handle *fwnode)
>  {
>  	struct fwnode_reference_args out_args;
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 064d6f5a06da4..b9a212bd12064 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -2000,6 +2002,53 @@ static int phylink_fill_available_pcs(struct phylink *pl,
>  	return ret;
>  }
>  
> +static int pcs_provider_notify(struct notifier_block *self,
> +			       unsigned long val, void *data)
> +{
> +	struct phylink *pl = container_of(self, struct phylink, fwnode_pcs_nb);
> +	struct fwnode_handle *pcs_fwnode = data;
> +	struct phylink_pcs *pcs;
> +
> +	rtnl_lock();

[Severity: High]
Can this unconditional lock acquisition cause a self-deadlock?

Because the provider addition is emitted via a synchronous blocking notifier, 
if a network driver or subsystem calls fwnode_pcs_add_provider() from a 
context that already holds rtnl_lock(), wouldn't this attempt to acquire 
the lock a second time?

> +
> +	/* Check if the just added PCS provider is
> +	 * in the phylink instance pcs-handle property.
> +	 */
> +	pcs = fwnode_phylink_pcs_get_from_fwnode(dev_fwnode(pl->config->dev),
> +						 pcs_fwnode);

[Severity: High]
Does this use the correct fwnode for multi-port devices like DSA switches?

It looks like pl->config->dev points to the parent switch's device, not the 
individual port. If the port-specific fwnode passed to phylink_create() isn't 
used here, will this search the switch's root device node instead and silently 
fail to find the pcs-handle?

> +	if (IS_ERR(pcs)) {
> +		rtnl_unlock();
> +		return NOTIFY_DONE;
> +	}
> +
> +	/* Add the PCS */
> +	mutex_lock(&pl->state_mutex);
> +
> +	/* Link PCS with phylink */
> +	list_add(&pcs->list, &pl->pcs_list);

[Severity: Critical]
Could this unconditional list addition corrupt the list if a single PCS 
provider is shared by multiple phylink instances?

If a single PCS provider is referenced by multiple ports on a switch, 
pcs_provider_notify() will trigger for each phylink instance. Since the 
list head inside struct phylink_pcs can only be part of one list at a time, 
won't the second list_add() corrupt the first phylink instance's pcs_list?

> +	pcs->phylink = pl;
> +
> +	/* Refresh supported interfaces */
> +	phy_interface_copy(pl->supported_interfaces,
> +			   pl->config->supported_interfaces);
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		phy_interface_or(pl->supported_interfaces,
> +				 pl->supported_interfaces,
> +				 pcs->supported_interfaces);

[Severity: High]
Is a call to phylink_validate() needed here to update the ethtool linkmodes?

This updates the supported interfaces mask with the PCS capabilities, but 
without recalculating pl->supported and pl->link_config.advertising, will 
the device fail to advertise or negotiate the new speeds and duplexes enabled 
by the late-attached PCS?

> +
> +	/* Force an interface reconfig if major config fail */
> +	if (pl->major_config_failed)
> +		pl->force_major_config = true;
> +
> +	mutex_unlock(&pl->state_mutex);
> +
> +	rtnl_unlock();
> +
> +	phylink_run_resolve(pl);
> +
> +	return NOTIFY_OK;
> +}
[ ... ]
> @@ -2124,6 +2173,12 @@ struct phylink *phylink_create(struct phylink_config *config,
>  	if (ret < 0)
>  		goto unlink_pcs_list;
>  
> +	/* Register notifier for late PCS attach */
> +	if (!phy_interface_empty(config->pcs_interfaces)) {
> +		pl->fwnode_pcs_nb.notifier_call = pcs_provider_notify;
> +		register_fwnode_pcs_notifier(&pl->fwnode_pcs_nb);
> +	}

[Severity: High]
Is there a race condition between the initial scan for available PCS 
providers and the registration of this late-attach notifier?

In phylink_create(), the initial scan happens here:

	ret = phylink_fill_available_pcs(pl, config);
	if (ret < 0)
		goto free_pl;

If a PCS provider driver probes and triggers the provider add notification 
after phylink_fill_available_pcs() but before register_fwnode_pcs_notifier() 
is called, won't the notification be permanently missed?

> +
>  	return pl;
>  
>  unlink_pcs_list:
> diff --git a/include/linux/pcs/pcs.h b/include/linux/pcs/pcs.h
> index b7cfdd680b2ab..45e8f96662db2 100644
> --- a/include/linux/pcs/pcs.h
> +++ b/include/linux/pcs/pcs.h
> @@ -4,7 +4,36 @@
>  
>  #include <linux/phylink.h>
>  
> +enum fwnode_pcs_notify_event {
> +	FWNODE_PCS_PROVIDER_ADD,
> +};

[Severity: Critical]
Is there a missing mechanism to handle PCS provider removal?

Without a corresponding remove event, when a PCS provider driver is unbound 
and frees the struct phylink_pcs, phylink is not notified. Since phylink 
retains the freed PCS pointer in pl->pcs_list, won't subsequent operations 
like phylink_resolve() iterate over the list and trigger a use-after-free?

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