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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing PCS removal notification leads to Use-After-Free when a PCS module is unloaded.
- [High] A TOCTOU race condition in `phylink_create()` can cause a newly added PCS provider to be permanently missed.
- [High] Unconditional `list_add()` in `pcs_provider_notify()` causes doubly-linked list corruption during concurrent additions.
- [High] Late PCS attachment fails for multi-port devices (e.g., DSA switches) due to incorrect fwnode lookup.
- [Medium] Synchronous acquisition of `rtnl_lock()` in the blocking notifier chain introduces a fragile self-deadlock risk.
--

--- Patch [5]: [PATCH net-next v9 05/12] net: phylink: support late PCS provider attach ---
commit 0738f7b893f648f2683558ff4efb6327359c5920
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: support late PCS provider attach

Add support for late PCS provider attachment to a phylink instance.
This works by creating a global notifier for the PCS provider and
making each phylink instance that makes use of fwnode subscribe to
this notifier.

[ ... ]

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 986e0c1e3d678..1d83b31874fb2 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -1999,6 +2001,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: Medium]
Does unconditional acquisition of rtnl_lock() here risk a deadlock?

If a network driver registers a PCS provider from a context that already
holds rtnl_lock() (which is common in network driver probe or open paths),
wouldn't the synchronous notifier callback attempt to recursively acquire
the lock and hang the system?

> +
> +	/* 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]
Will this lookup fail for multi-port devices like DSA switches?

It seems to fetch the fwnode from the parent device (pl->config->dev) rather
than the port-specific fwnode passed during phylink creation. If the
pcs-handle property is in the port's subnode, will this return an error and
silently abort the late attachment?

> +	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: High]
Could this corrupt the doubly-linked list if the PCS is already present?

If a PCS provider is added concurrently, it might already be picked up by
phylink_fill_available_pcs() and then passed to this notifier. Calling
list_add() unconditionally on a node already in the list overwrites its
pointers and causes list corruption.

> +	pcs->phylink = pl;
> +

[ ... ]

> @@ -2116,6 +2165,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 fetching available PCS providers and
registering this notifier?

In phylink_create(), the list of available PCS providers is fetched earlier
via phylink_fill_available_pcs(). If a PCS provider calls
fwnode_pcs_add_provider() exactly after the list is fetched but before
register_fwnode_pcs_notifier() is called here, wouldn't the provider be
permanently missed since it wasn't in the global list initially and the
notification was 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]
Does the lack of a removal notification event create a use-after-free risk?

If a PCS provider module is unloaded, fwnode_pcs_del_provider() is called to
free internal state, but phylink is never notified because there is no
FWNODE_PCS_PROVIDER_DEL event.

Won't the freed pointer remain linked in pl->pcs_list and be dereferenced
during the next phylink_resolve() run triggered by a timer or link state
change?

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