Re: [PATCH net-next v7 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:
- [High] Use-After-Free in PCS global notifier list due to missing unregister in `phylink_create()` error paths.
- [High] NULL pointer dereference due to uninitialized `pl->config` in the PCS notifier callback.
- [High] List corruption race between `phylink_create()` and `fwnode_pcs_add_provider()`.
- [High] Use-After-Free of `phylink_pcs` structure in `pcs_provider_notify()` due to missing reference counting.
- [Medium] TOCTOU race condition in `phylink_create()` causes missed PCS attachments.
--

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

net: phylink: support late PCS provider attach

This patch adds support for late PCS provider attachment by introducing a
global notifier. Phylink instances subscribe to this notifier and react to
FWNODE_PCS_PROVIDER_ADD events by linking the new PCS and refreshing
supported interfaces.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index ca4dad4b140a0..0734c98498a99 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -1997,6 +1999,51 @@ 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;
> +
> +	/* 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]
Could this cause a NULL pointer dereference? 

The notifier is registered in phylink_create() before pl->config is 
initialized. If a concurrent thread adds a PCS provider immediately after 
registration, won't pcs_provider_notify() attempt to access pl->config->dev 
while it is still NULL?

> +	if (IS_ERR(pcs))
> +		return NOTIFY_DONE;
> +
> +	/* Add the PCS */
> +	rtnl_lock();

[Severity: High]
Does this code risk a use-after-free? 

The phylink_pcs pointer is retrieved without incrementing a reference count, 
and then the thread can sleep while waiting for rtnl_lock(). 

If the module providing the PCS is concurrently unbound during this wait, 
freeing the phylink_pcs structure, will this code dereference the freed 
pointer once the lock is acquired?

> +
> +	mutex_lock(&pl->state_mutex);
> +
> +	/* Link PCS with phylink */
> +	list_add(&pcs->list, &pl->pcs_list);

[Severity: High]
Is it possible for the list to become corrupted here? 

If phylink_fill_available_pcs() finds the newly added PCS and links it to 
pl->pcs_list during phylink_create(), and then the delayed 
FWNODE_PCS_PROVIDER_ADD notification fires right after, won't 
pcs_provider_notify() unconditionally add the exact same pcs structure 
to the list again?

[ ... ]

> @@ -2068,6 +2115,12 @@ struct phylink *phylink_create(struct phylink_config *config,
>  				 pl->supported_interfaces,
>  				 pcs->supported_interfaces);
>  
> +	/* 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: Medium]
Is there a race condition window here? 

If a new PCS provider is added concurrently between the earlier call to 
phylink_fill_available_pcs() and the registration of the notifier here, 
will the FWNODE_PCS_PROVIDER_ADD event fire before the notifier is active, 
causing the PCS to be permanently missed?

>  	pl->config = config;
>  	if (config->type == PHYLINK_NETDEV) {
>  		pl->netdev = to_net_dev(config->dev);

[Severity: High]
If phylink_create() encounters an error later in the function (like an 
invalid config->type or phylink_parse_mode() failing), does this leave 
a dangling pointer in the global notifier chain since 
unregister_fwnode_pcs_notifier() isn't called before kfree(pl)?

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