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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing capability recalculation of `pl->supported` after a late PCS is attached.
- [High] Data race on `pl->supported_interfaces` due to missing `rtnl_lock()` in `pcs_provider_notify()`.
- [High] List corruption in `phylink_add_pcs()` when a PCS is shared across multiple phylink instances.
--

commit 5584b5ab8035c3438b3834406ac1b9de7eaf5243
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: support late PCS provider attach

Add support for late PCS provider attachment to a phylink instance.
Similar to FWNODE_PCS_PROVIDER_DEL, FWNODE_PCS_PROVIDER_ADD is added to
address the case where a PCS provider is added after a phylink instance is
created and started.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index e88c1effe880f..a68d77ba57282 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1926,6 +1926,27 @@ int phylink_set_fixed_link(struct phylink *pl,
>  }
>  EXPORT_SYMBOL_GPL(phylink_set_fixed_link);
>  
> +static void phylink_add_pcs(struct phylink *pl, struct phylink_pcs *pcs)
> +{
> +	struct phylink_pcs *tmp;
> +
> +	/*
> +	 * Make sure state mutex is locked to protect concurrent
> +	 * access to phylink instance PCS list from
> +	 * initial fill_available_pcs and late PCS attach
> +	 */
> +	lockdep_assert_held(&pl->state_mutex);
> +
> +	list_for_each_entry(tmp, &pl->pcs_list, list)
> +		if (tmp == pcs)
> +			return;
> +
> +	list_add_tail(&pcs->list, &pl->pcs_list);
> +
> +	/* Link PCS to phylink */
> +	pcs->phylink = pl;
> +}

[Severity: High]
Does this code risk corrupting the list if a PCS is shared across multiple
phylink instances?  If a misconfigured device tree maps multiple MACs to the
same PCS node, multiple phylink instances could call phylink_add_pcs() with the
same phylink_pcs pointer.

Would list_add_tail() unconditionally overwrite the pcs->list pointers and
destroy the list integrity of the first phylink instance that claimed it?
Should there be a check to ensure pcs->phylink is not already assigned to
another instance?

[ ... ]

> @@ -2004,27 +2025,57 @@ static int pcs_provider_notify(struct notifier_block *self,
>  	struct fwnode_pcs_provider *pp = data;
>  	struct phylink_pcs *pcs, *tmp;
>  	bool resolve = false;
> +	int count, i;
>  
> -	rtnl_lock();
> +	/*
> +	 * On PCS provider deletion hold rtnl lock as one of
> +	 * PCS can be currently in use by the phylink instance
> +	 * and ethtool OPs can reference it.
> +	 */
> +	if (val == FWNODE_PCS_PROVIDER_DEL)
> +		rtnl_lock();

[Severity: High]
Does skipping rtnl_lock() for FWNODE_PCS_PROVIDER_ADD introduce a data race?

Further down in pcs_provider_notify(), the code updates
pl->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);

Since pl->supported_interfaces is mutated without rtnl_lock(), could a
concurrent ethtool command or SFP hotplug calling phylink_validate_mask()
(which runs under rtnl_lock() and iterates over pl->supported_interfaces) read
a torn or partially empty mask, leading to spurious capability validation
failures?

[ ... ]

> @@ -2038,7 +2089,8 @@ static int pcs_provider_notify(struct notifier_block *self,
>  
>  	mutex_unlock(&pl->state_mutex);
>  
> -	rtnl_unlock();
> +	if (val == FWNODE_PCS_PROVIDER_DEL)
> +		rtnl_unlock();
>  
>  	phylink_run_resolve(pl);
>  

[Severity: High]
Is a capability recalculation of pl->supported missing after a late PCS is
attached?

In pcs_provider_notify(), pl->supported_interfaces is updated when a new PCS
is attached (just above the mutex_unlock() shown here). However, it doesn't
appear that phylink_validate() is called to update pl->supported and
pl->link_config.advertising to reflect the new interfaces. 

Could this cause new interfaces supported by a late-attached PCS to be omitted
from pl->supported, resulting in SFP hotplug validation failures and ethtool
hiding the capabilities?

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