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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Lockless access to `pcs->phylink` in `phylink_pcs_change()` races with `phylink_destroy()`, leading to a use-after-free.
- [Medium] Concurrent lockless read of `pl->supported_interfaces` races with modification in `pcs_provider_notify()`, causing spurious validation failures.
--

commit 9d64e413cfd8c4a13418afd3ec905227f98db552
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: support PCS provider release

This commit adds support for the release of fwnode PCS from a PCS provider.
It implements a global notifier that phylink instances subscribe to, allowing
them to dynamically remove released PCS interfaces and reconfigure the link.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 96f72eb5a6fad..e88c1effe880f 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

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

[Severity: Medium]
Does this non-atomic modification of pl->supported_interfaces risk racing
with lockless readers?

In pcs_provider_notify(), pl->supported_interfaces is overwritten and
rebuilt while holding only pl->state_mutex. However, concurrent callers
like phylink_validate() read pl->supported_interfaces holding only
rtnl_lock():

drivers/net/phy/phylink.c:phylink_validate() {
    ...
    const unsigned long *interfaces = pl->supported_interfaces;

    if (state->interface == PHY_INTERFACE_MODE_NA)
        return phylink_validate_mask(pl, NULL, supported, state,
                                     interfaces);

    if (!test_bit(state->interface, interfaces))
        return -EINVAL;
    ...
}

If phylink_validate() runs between the phy_interface_copy() and the
completion of the phy_interface_or() loop, could it observe an incomplete
bitmap missing the PCS interfaces and spuriously reject a valid link mode
with -EINVAL?

[ ... ]

> @@ -2102,14 +2221,22 @@ void phylink_destroy(struct phylink *pl)
>  	if (pl->link_gpio)
>  		gpiod_put(pl->link_gpio);
>  
> +	/* Unregister notifier for late PCS attach */
> +	if (pl->fwnode_pcs_nb.notifier_call)
> +		unregister_fwnode_pcs_notifier(&pl->fwnode_pcs_nb);
> +
>  	cancel_work_sync(&pl->resolve);
>  
> +	mutex_lock(&pl->state_mutex);
> +
>  	/* Remove every PCS from phylink PCS list */
>  	list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list) {
>  		pcs->phylink = NULL;
>  		list_del(&pcs->list);
>  	}
>  
> +	mutex_unlock(&pl->state_mutex);
> +
>  	fwnode_handle_put(pl->fwnode);
>  
>  	kfree(pl);

[Severity: High]
Could clearing pcs->phylink here and subsequently freeing pl race with
lockless readers?

The phylink_pcs_change() function is typically invoked asynchronously from
a PCS hardware interrupt and reads pcs->phylink locklessly:

drivers/net/phy/phylink.c:phylink_pcs_change() {
    ...
    struct phylink *pl = pcs->phylink;

    /*
     * Ignore PCS link state change if the PCS is not
     * attached to a phylink instance or the phylink
     * instance is not currently using this PCS.
     */
    if (!pl || READ_ONCE(pl->pcs) != pcs)
        return;

    phylink_link_changed(pl, up, "pcs");
    ...
}

If a MAC driver is unbound concurrently, phylink_destroy() clears
pcs->phylink and frees pl without an RCU grace period or synchronization
with concurrent readers.

If phylink_pcs_change() is preempted after reading the pointer but before
dereferencing it (e.g., when evaluating READ_ONCE(pl->pcs)), could this
lead to a use-after-free regression?

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