Re: [PATCH net-next v6 05/12] 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] Use-After-Free due to missing notifier unregistration when phylink instance is destroyed.
- [High] Use-After-Free due to incomplete PCS detachment logic when the phylink instance is stopped.
- [High] Data race between `pcs_provider_notify()` and asynchronous readers leads to `pl->pcs_list` corruption.
- [Medium] Memory leak of `fwnode_handle` references in `fwnode_phylink_pcs_get_from_fwnode()`.
--

commit e4be489886d6b3bd45151876412cae4ab8aa7956
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. It implements a global notifier for the PCS provider, allowing
phylink instances to subscribe and attach the PCS when it becomes available.

> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> index 72f56f55d198d..911d95cf1b09c 100644
> --- a/drivers/net/pcs/pcs.c
> +++ b/drivers/net/pcs/pcs.c

[ ... ]

> @@ -147,6 +158,35 @@ struct phylink_pcs *fwnode_pcs_get(struct fwnode_handle *fwnode, int index)
>  }
>  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)
> +			break;

[Severity: Medium]
Does this leak a fwnode_handle reference?

fwnode_parse_pcsspec() internally acquires a reference to pcsspec.fwnode.
However, on both the break path (when a match is found) and the loop
continuation path, this reference is never released with
fwnode_handle_put().

Will this leak a reference for every successfully parsed PCS specification?

> +
> +		/* Check the next PCS reference */
> +		index++;
> +	}
> +
> +	return fwnode_pcs_get(fwnode, index);
> +}
> +EXPORT_SYMBOL_GPL(fwnode_phylink_pcs_get_from_fwnode);

[ ... ]

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index b45bee9273007..65b67c33e12fd 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -1998,6 +2000,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);
> +	if (IS_ERR(pcs))
> +		return NOTIFY_DONE;
> +
> +	/* Add the PCS */
> +	rtnl_lock();
> +
> +	list_add(&pcs->list, &pl->pcs_list);

[Severity: High]
Is it safe to mutate this list while only holding the rtnl_lock?

pcs_provider_notify() adds a new PCS to pl->pcs_list under the
rtnl_lock().

However, asynchronous readers like phylink_major_config() executed from
the phylink_resolve workqueue traverse this same list using
list_for_each_entry() while only holding pl->state_mutex.

Since list_add() is not atomic and the locks are disjoint, can concurrent
access cause readers to traverse a partially updated list structure?

> +
> +	/* Link phylink if we are started */
> +	if (!pl->phylink_disable_state)
> +		pcs->phylink = pl;

[Severity: High]
Does this create a use-after-free when a phylink instance is stopped?

When the interface is stopped, phylink_stop() clears pcs->phylink by
setting it to NULL for every entry in pl->pcs_list.

If the underlying PCS driver is unbound while stopped, it calls
phylink_release_pcs(), which checks if pcs->phylink is NULL and returns
early, failing to remove the PCS from pl->pcs_list:

phylink_release_pcs() {
    pl = pcs->phylink;
    if (!pl)
        return;
    list_del(&pcs->list);
}

Will this leave a freed PCS structure dangling in pl->pcs_list which
could be traversed when phylink_start() is called later?

> +
> +	/* 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);

[ ... ]

> @@ -2053,6 +2100,11 @@ struct phylink *phylink_create(struct phylink_config *config,
>  				 pl->supported_interfaces,
>  				 pcs->supported_interfaces);
>  
> +	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]
If the phylink instance is destroyed, is this notifier ever unregistered?

If phylink_destroy() frees the phylink instance but doesn't unregister
the notifier block, will it leave a dangling pointer in the global
fwnode_pcs_notify_list? If so, won't this cause a kernel panic when the
next PCS provider is added?

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

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