Re: [PATCH net-next v16 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:
- [Medium] phylink: stale linkmodes in pl->supported after PCS hot-unplug
- [Medium] phylink: torn reads of pl->supported_interfaces during pcs notify
--

commit 729bf8a9cf1a1b007e4c318df0f8ae3838564ec2
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/phy/phylink.c b/drivers/net/phy/phylink.c
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -1939,11 +1964,89 @@ static int phylink_fill_available_pcs(struct phylink *pl,
> +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);

[ ... ]

> +	/* 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 code leave the ethtool linkmodes in a stale state?

When a PCS is removed, this block refreshes pl->supported_interfaces, but it
does not recalculate pl->supported and pl->link_config.advertising via
phylink_validate(). 

Could this result in ethtool continuing to report capabilities for a
dynamically removed PCS, leading to userspace attempting to configure
unsupported modes?

[Severity: Medium]
Is this multi-step update of pl->supported_interfaces safe from concurrent
readers?

The update copies the interface bits and then ORs each PCS in multiple
steps. If this happens during an event (such as FWNODE_PCS_PROVIDER_ADD)
where only state_mutex is held without rtnl_lock, it might expose an
intermediate, torn state to concurrent readers.

For instance, a concurrent thread could read pl->supported_interfaces in
phylink_validate() while holding only rtnl_lock:

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;
}

Could this data race cause readers to observe an incomplete mask and
spuriously reject valid SFP modules or ethtool configurations?

> +
> +	mutex_unlock(&pl->state_mutex);
> +
> +	rtnl_unlock();
> +
> +	phylink_run_resolve(pl);
> +
> +	return NOTIFY_OK;
> +}

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