Re: [PATCH net-next v11 05/12] net: phylink: support PCS provider release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] A self-deadlock (AA deadlock) occurs on `fwnode_pcs_mutex` when a PCS provider is deleted.
- [High] Unconditional `list_del()` in `phylink_del_pcs()` leads to a NULL pointer dereference or list corruption if the PCS was never added to the phylink's PCS list.
- [High] Lockless iterations of `pl->pcs_list` race with concurrent dynamic deletion via `phylink_del_pcs()`, causing `LIST_POISON` dereferences.
- [High] Lockless access to `pl->pcs` in ethtool callbacks races with runtime PCS deletion, leading to Use-After-Free.
--

--- Patch [5]: [PATCH net-next v11 05/12] net: phylink: support PCS provider release ---
commit a1d43d116ac4a79cbb3a27b395cf89f68d773799
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: support PCS provider release
    
Add support for release of fwnode PCS from a PCS provider.
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/pcs/pcs.c b/drivers/net/pcs/pcs.c
> --- a/drivers/net/pcs/pcs.c
> +++ b/drivers/net/pcs/pcs.c

[ ... ]

> @@ -67,6 +80,11 @@ void fwnode_pcs_del_provider(struct fwnode_pcs_provider *pp)
>  
>  	mutex_lock(&fwnode_pcs_mutex);
>  
> +	/* Signal phylink to release any PCS from this provider */
> +	blocking_notifier_call_chain(&fwnode_pcs_notify_list,
> +				     FWNODE_PCS_PROVIDER_DEL,
> +				     pp->fwnode);
> +

[Severity: High]
Does this sequence cause a self-deadlock? fwnode_pcs_del_provider()
acquires the global fwnode_pcs_mutex and then broadcasts the
FWNODE_PCS_PROVIDER_DEL event via blocking_notifier_call_chain().

This synchronously invokes the phylink callback pcs_provider_notify(),
which calls fwnode_phylink_pcs_get_from_fwnode(). That delegates to
fwnode_pcs_get() -> __fwnode_pcs_get() -> fwnode_pcs_get_from_pcsspec().

Inside fwnode_pcs_get_from_pcsspec(), the code attempts to re-acquire
the exact same fwnode_pcs_mutex. Since it is a non-recursive mutex already
held by this thread, could this hang the system?

>  	list_del(&pp->link);
>  	fwnode_dev_initialized(pp->fwnode, false);
>  	fwnode_handle_put(pp->fwnode);

[ ... ]

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> +static void phylink_del_pcs(struct phylink *pl, struct phylink_pcs *pcs)
> +{
> +	list_del(&pcs->list);

[Severity: High]
Can this lead to a NULL pointer dereference or list corruption?

In phylink_create(), the PCS notifier is registered before the initial
PCS list is populated. If a FWNODE_PCS_PROVIDER_DEL event fires concurrently
during this window, pcs_provider_notify() will look up the relevant PCS
and unconditionally call phylink_del_pcs().

Because the PCS has not yet been linked into pl->pcs_list, its list_head
would be uninitialized or zero-initialized. Executing list_del(&pcs->list)
on an unlinked node dereferences NULL pointers.

> +	pcs->phylink = NULL;
> +
> +	/*
> +	 * Check if we are removing the PCS currently
> +	 * in use by this phylink instance. If this is the case,
> +	 * tear down the link, force phylink resolve to reconfigure the
> +	 * interface mode, disable the current PCS and set the
> +	 * phylink PCS to NULL.
> +	 */
> +	if (pl->pcs == pcs) {
> +		if (pl->old_link_state) {
> +			phylink_link_down(pl);
> +			pl->old_link_state = false;
> +		}
> +		if (pl->cfg_link_an_mode == MLO_AN_INBAND)
> +			timer_delete_sync(&pl->link_poll);
> +		phylink_pcs_disable(pl->pcs);
> +
> +		pl->force_major_config = true;
> +		WRITE_ONCE(pl->pcs, NULL);

[Severity: High]
Does clearing pl->pcs here race with ethtool callbacks, leading to a
use-after-free?

Functions triggered by ethtool, such as phylink_ethtool_nway_reset(),
operate with only the RTNL lock held and do not acquire pl->state_mutex.
They call phylink_pcs_an_restart(), which reads pl->pcs and
dereferences its operations pointer locklessly:

if (pl->pcs && ... )
    pl->pcs->ops->pcs_an_restart(pl->pcs);

Concurrently, this teardown path takes pl->state_mutex (but not RTNL),
detaches the PCS, and returns. The underlying memory can then be freed
by the provider driver. Could the ethtool path read a valid pl->pcs
pointer just before detachment, and dereference it after it is freed?

> +	}
> +}
> +
> +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);
> +	const struct fwnode_handle *pl_fwnode = pl->fwnode;
> +	struct fwnode_handle *pcs_fwnode = data;
> +	struct phylink_pcs *pcs;
> +
> +	/* Check if the PCS provider is relevant for this phylink instance */
> +	pcs = fwnode_phylink_pcs_get_from_fwnode(pl_fwnode, pcs_fwnode);
> +	if (IS_ERR(pcs))
> +		return NOTIFY_DONE;
> +
> +	mutex_lock(&pl->state_mutex);
> +
> +	phylink_del_pcs(pl, pcs);

[Severity: High]
Does dynamically modifying the list here race with existing lockless
iterations of pl->pcs_list?

Before this patch, pl->pcs_list was populated in phylink_create() and
treated as read-only. Existing readers like phylink_validate_mac_and_pcs()
and phylink_inband_caps() iterate over pl->pcs_list locklessly while
holding only the RTNL lock.

If a PCS provider is removed concurrently under pl->state_mutex, could
these RTNL-only readers follow poisoned list pointers resulting from
list_del(), crashing the kernel?

Additionally, the error path in phylink_create() iterates and unlinks
the list locklessly before unregistering the global notifier:

unlink_pcs_list:
    list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list) {
        list_del(&pcs->list);
        ...
    }

Could this error path race with the notifier callback?

> +
> +	/* Refresh supported interfaces */
> +	phy_interface_copy(pl->supported_interfaces,
> +			   pl->config->supported_interfaces);

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