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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing unregistration of the global PCS notifier on `phylink_create` error paths leads to a Use-After-Free.
- [High] Premature registration of the PCS notifier before `pl->config` is initialized allows a NULL pointer dereference.
- [High] Unprotected list iteration and data race on `supported_interfaces` in `phylink_create()`.
- [High] `pcs_provider_notify()` queries the wrong firmware node for multi-port devices, breaking late PCS attachment.
--

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

net: phylink: support late PCS provider attach
    
This commit adds support for late PCS provider attachment to a phylink
instance by creating a global notifier chain that emits events when a new
PCS provider is added. Phylink instances subscribe to this notifier to
dynamically link PCS providers and trigger interface reconfiguration when
they appear.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 562e09d1838b0..d38820543ae4a 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -1988,21 +2011,70 @@ static int phylink_fill_available_pcs(struct phylink *pl,
> +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();
> +
> +	/* 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 correctly handle multi-port MACs or DSA switches where the pcs-handle
property is located in a child port's firmware node?

In phylink_create(), the port-specific firmware node is passed in as the
fwnode parameter. However, this callback queries the top-level
dev_fwnode(pl->config->dev), which may not contain the pcs-handle property
for individual ports. 

Should the instance store the port's fwnode during creation so the notifier
can use it here instead?

[ ... ]

> @@ -2047,15 +2119,21 @@ struct phylink *phylink_create(struct phylink_config *config,
>  	INIT_WORK(&pl->resolve, phylink_resolve);
>  	INIT_LIST_HEAD(&pl->pcs_list);
>  
> +	/*
> +	 * First register notifier for late PCS attach to catch
> +	 * any possible provider that registers while we fill
> +	 * the available PCS.
> +	 */
> +	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 here if a PCS provider is registered concurrently?

By registering the global notifier here, pcs_provider_notify() can be invoked
immediately on another CPU. That callback dereferences pl->config->dev:

pcs_provider_notify() {
   ...
   pcs = fwnode_phylink_pcs_get_from_fwnode(dev_fwnode(pl->config->dev),
                                            pcs_fwnode);
}

However, pl->config isn't assigned until further down in phylink_create().
Could this result in a NULL pointer dereference if a driver probes
concurrently?

>  	/* Fill the PCS list with available PCS from phylink config */
>  	ret = phylink_fill_available_pcs(pl, config);
>  	if (ret < 0)
>  		goto free_pl;

[Severity: High]
If phylink_fill_available_pcs() or subsequent initialization steps fail, will
this leak the notifier block into the global chain?

The error handling path at free_pl calls kfree(pl). If the notifier was
registered right above this step, but isn't unregistered before freeing pl,
could the next system-wide PCS registration trigger a use-after-free and crash
when it walks the global fwnode_pcs_notify_list?

> -	/* Link available PCS to phylink */
> -	list_for_each_entry(pcs, &pl->pcs_list, list)
> -		pcs->phylink = pl;
> -
>  	phy_interface_copy(pl->supported_interfaces,
>  			   config->supported_interfaces);
>  
> +	/* Update 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]
Does this list iteration need to be protected by pl->state_mutex?

Since the PCS notifier has already been registered globally above,
pcs_provider_notify() could be executing on another CPU, acquiring
state_mutex, and modifying both pl->pcs_list and pl->supported_interfaces.

Could iterating the list and doing the bitwise OR here without holding the lock
lead to data corruption?

[ ... ]

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