Re: [PATCH net-next v18 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:
- [Critical] phylink: self-deadlock in phylink_update_mac_pause_capabilities()
- [High] phylink: data race on link_config capabilities in pcs_provider_notify()
- [Medium] phylink: pcs_provider_notify() silently destroys user-configured ethtool settings
- [Medium] phylink: data race during phylink_create() due to early notifier registration
--

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

net: phylink: support PCS provider release

This patch adds support for releasing a fwnode PCS from a PCS provider.
It creates a global notifier for the PCS provider, allowing phylink
instances using fwnode to subscribe and dynamically handle PCS removal.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 9441afe372c33..019ce3d89da04 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -545,11 +547,15 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
>  	unsigned long capabilities;
>  	int ret;
>  
> +	mutex_lock(&pl->state_mutex);

[Severity: Critical]
Does this new lock acquisition cause a self-deadlock?

`phylink_update_mac_pause_capabilities()` explicitly acquires
`pl->state_mutex` and then calls `phylink_validate()`, which in turn calls
`phylink_validate_mac_and_pcs()`.

Because `struct mutex` is not recursive, attempting to acquire it again from
the same thread here results in a guaranteed self-deadlock.

This will hang the system for any network driver that dynamically updates MAC
pause capabilities at runtime.

[ ... ]

> +static int pcs_provider_notify(struct notifier_block *self,
> +			       unsigned long val, void *data)
> +{
[ ... ]
> +	/* 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);
> +
> +	mutex_unlock(&pl->state_mutex);
> +
> +	/* Recalculate capabilities */
> +	linkmode_fill(pl->supported);
> +	linkmode_fill(pl->link_config.advertising);

[Severity: High]
Does dropping `state_mutex` here introduce a data race?

While the mutex is dropped, `linkmode_fill()` is executed on
`pl->link_config.advertising`, temporarily flooding the link advertisement mask
with 1s to act as a generic starting point for validation.

The validation spans multiple nested lock/unlock cycles, leaving the structure
in an invalid state for a prolonged window.

Concurrently, the `phylink_resolve()` workqueue executes without `rtnl_lock()`.
It acquires the now-free `state_mutex` and reads `pl->link_config`.

`phylink_resolve()` will copy the torn or artificially fully-set advertisement
mask and propagate it to the hardware, potentially resulting in undefined
behavior or hardware crashes.

[Severity: Medium]
Will this unconditionally overwrite user-configured ethtool settings?

Users can restrict link modes (e.g., disabling Gigabit speeds) via ethtool,
which permanently modifies `pl->link_config.advertising`.

When a PCS provider is removed, executing `linkmode_fill()` before calling
`phylink_validate_mask()` resets the advertisement mask to the maximum
capabilities of the hardware.

This entirely wipes out any manual ethtool restrictions applied by the user
prior to the PCS removal.

[ ... ]

> @@ -2152,10 +2261,18 @@ struct phylink *phylink_create(struct phylink_config *config,
>  	__set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
>  	timer_setup(&pl->link_poll, phylink_fixed_poll, 0);
>  
> +	/* First register notifier for hotplug PCS events */
> +	if (!phy_interface_empty(config->pcs_interfaces)) {
> +		pl->fwnode_pcs_nb.notifier_call = pcs_provider_notify;
> +		register_fwnode_pcs_notifier(&pl->fwnode_pcs_nb);
> +	}
> +
>  	/* Fill the PCS list with available PCS from phylink config */
>  	ret = phylink_fill_available_pcs(pl, config);
[ ... ]
> @@ -2170,13 +2287,15 @@ struct phylink *phylink_create(struct phylink_config *config,
>  				 pl->supported_interfaces,
>  				 pcs->supported_interfaces);
>  
> +	mutex_unlock(&pl->state_mutex);
> +
>  	linkmode_fill(pl->supported);
>  	linkmode_copy(pl->link_config.advertising, pl->supported);

[Severity: Medium]
Is it safe to register the notifier before the capability bitmaps are
initialized?

By calling `register_fwnode_pcs_notifier()` early, the phylink instance becomes
visible to asynchronous hotplug events before initialization finishes.

A concurrent `FWNODE_PCS_PROVIDER_DEL` event on another CPU could trigger
`pcs_provider_notify()`, which recalculates and mutates `pl->supported` and
`pl->link_config` under `rtnl_lock()`.

Meanwhile, `phylink_create()` continues executing `linkmode_fill()` and
`linkmode_copy()` on those same fields without holding `rtnl_lock()` or
`state_mutex`.

This unsynchronized data race on the capability bitmaps can corrupt the initial
link modes.

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