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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Self-deadlock in fwnode_pcs_del_provider via FWNODE_PCS_PROVIDER_DEL notification.
- [High] Error paths in phylink_create skip necessary cleanup, leading to Use-After-Free of the phylink instance and dangling PCS pointers.
- [High] phylink_del_pcs unconditionally calls list_del, crashing on PCS structures not tracked in the phylink list.
- [High] Missing reference counting on struct phylink_pcs pointers allows a TOCTOU Use-After-Free during asynchronous provider removal.
--

--- Patch [6]: [PATCH net-next v11 06/12] net: phylink: support late PCS provider attach ---
commit d404b5bc48ec98384e8bad4f7b0b1af967121eee
Author: Christian Marangi <[email protected]>

net: phylink: support late PCS provider attach

This patch adds support for attaching a PCS provider to a phylink instance
after the instance has been created and started. It introduces
FWNODE_PCS_PROVIDER_ADD to notify phylink when a new PCS provider becomes
available, linking it and triggering a resolve step to recover links.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 548e783ae2aaa..69d4f6675db0f 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -1939,7 +1960,7 @@ static int phylink_fill_available_pcs(struct phylink *pl,
>  		if (!pcs)
>  			continue;
>  
> -		list_add_tail(&pcs->list, &pl->pcs_list);
> +		phylink_add_pcs(pl, pcs);

[Severity: High]
Could the lack of reference counting on the returned phylink_pcs pointers
lead to a use-after-free here?

fwnode_pcs_get() and its underlying provider callbacks return a pointer
without taking a reference. If a concurrent provider unbind occurs right
before pl->state_mutex is acquired, the provider could be removed and the
memory freed.

When the lock is finally acquired in phylink_fill_available_pcs(), could
this add a freed pointer to the list?

>  	}
>  
>  	mutex_unlock(&pl->state_mutex);
> @@ -1991,7 +2012,19 @@ static int pcs_provider_notify(struct notifier_block *self,
>  
>  	mutex_lock(&pl->state_mutex);
>  
> -	phylink_del_pcs(pl, pcs);
> +	switch (val) {
> +	case FWNODE_PCS_PROVIDER_ADD:
> +		phylink_add_pcs(pl, pcs);
> +
> +		/* Force an interface reconfig if major config fail */
> +		if (pl->major_config_failed)
> +			pl->force_major_config = true;
> +
> +		break;
> +	case FWNODE_PCS_PROVIDER_DEL:
> +		phylink_del_pcs(pl, pcs);

[Severity: High]
Will this cause a crash if the PCS being removed was never actually added to
the list?

When handling a provider removal, phylink_del_pcs() is called
unconditionally based purely on its presence in the device tree. If the
driver's fill_available_pcs() omitted it, or if phylink_create() failed
before it could be added, the pcs->list remains uninitialized.

Would this lead to list_del(&pcs->list) dereferencing a NULL pointer?

[Severity: Critical]
Could this notifier chain lead to a self-deadlock when removing a provider?

When fwnode_pcs_del_provider() is called, it acquires fwnode_pcs_mutex and
fires the FWNODE_PCS_PROVIDER_DEL notification.

This notifier callback, pcs_provider_notify(), responds by calling
fwnode_phylink_pcs_get_from_fwnode():

pcs_provider_notify()
   pcs = fwnode_phylink_pcs_get_from_fwnode(pl_fwnode, pcs_fwnode);

This subsequently calls fwnode_pcs_get_from_pcsspec(), which attempts to
acquire the exact same fwnode_pcs_mutex lock.

Since the lock is not recursive, will this result in a deadlock on the
thread removing the provider?

> +		break;
> +	}
>  
>  	/* Refresh supported interfaces */
>  	phy_interface_copy(pl->supported_interfaces,
> @@ -2077,10 +2110,6 @@ struct phylink *phylink_create(struct phylink_config *config,
>  
>  	mutex_lock(&pl->state_mutex);
>  
> -	/* Link available PCS to phylink */
> -	list_for_each_entry(pcs, &pl->pcs_list, list)
> -		pcs->phylink = pl;
> -

[Severity: High]
Does the error handling in phylink_create() leak dangling pointers and use
freed memory if a failure occurs after the notifier is registered?

The PCS notifier is registered early in phylink_create(). If an asynchronous
FWNODE_PCS_PROVIDER_ADD event fires concurrently, pcs_provider_notify()
can queue the pl->resolve work item.

If phylink_create() subsequently fails (for example, in
phylink_fill_available_pcs()), the error path jumps to unregister_pcs_notify
and frees the phylink structure:

unregister_pcs_notify:
    unregister_fwnode_pcs_notifier(&pl->fwnode_pcs_nb);
free_pl:
    kfree(pl);

This path does not call cancel_work_sync(&pl->resolve), leaving the queued
work to execute on freed memory.

Additionally, because the error path skips unlinking the PCS list, any PCS
dynamically added by the notifier will be left with a dangling pcs->phylink
pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6