Re: [PATCH net-next v7 03/12] net: phylink: add phylink_release_pcs() to externally release a PCS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Lockless modification of `pl->pcs_list` races with the `phylink_resolve` workqueue, leading to list corruption and use-after-free.
- [High] The active PCS is disabled prematurely before the MAC link is brought down, violating the standard teardown sequence.
- [Medium] `pl->supported` link modes are not updated when `pl->supported_interfaces` shrinks, leading to stale ethtool capabilities.
- [Medium] `pl->sfp_interfaces` is not updated when a PCS is released.
--

commit 0240edfdad0af399cf16b6105ab8d1a7a62254fb
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: add phylink_release_pcs() to externally release a PCS

Add phylink_release_pcs() to externally release a PCS from a phylink
instance. This can be used to handle case when a single PCS needs to be
removed and the phylink instance needs to be refreshed.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index cb07184ce82f3..ca4dad4b140a0 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -918,6 +919,58 @@ static void phylink_resolve_an_pause(struct phylink_link_state *state)
>  	}
>  }
>  
> +/**
> + * phylink_release_pcs - Removes a PCS from the phylink PCS available list
> + * @pcs: a pointer to the phylink_pcs struct to be released
> + *
> + * This function release a PCS from the phylink PCS available list if
> + * actually in use. It also refreshes the supported interfaces of the
> + * phylink instance by copying the supported interfaces from the phylink
> + * conf and merging the supported interfaces of the remaining available PCS
> + * in the list and trigger a resolve.
> + */
> +void phylink_release_pcs(struct phylink_pcs *pcs)
> +{
> +	struct phylink *pl;
> +
> +	ASSERT_RTNL();
> +
> +	pl = pcs->phylink;
> +	if (!pl)
> +		return;
> +
> +	list_del(&pcs->list);
> +	pcs->phylink = NULL;
> +
> +	mutex_lock(&pl->state_mutex);

[Severity: High]
Does this list_del() race with the phylink_resolve() workqueue?

Since phylink_resolve() does not hold rtnl_lock, it takes pl->state_mutex
and traverses pl->pcs_list in phylink_major_config().

If list_del() executes completely locklessly with respect to the
phylink_resolve() traversal, will the iterator follow poisoned list pointers,
resulting in a page fault and kernel panic?

Also, in phylink_destroy(), does clearing the list before canceling the resolve
workqueue without state_mutex open another identical race window?

> +
> +	/* Check if we are removing the PCS currently
> +	 * in use by phylink. If this is the case,
> +	 * force phylink resolve to reconfigure the interface
> +	 * mode, disable the current PCS and set the
> +	 * phylink PCS to NULL.
> +	 */
> +	if (pl->pcs == pcs) {
> +		phylink_pcs_disable(pl->pcs);

[Severity: High]
Is it safe to disable the active PCS here before the MAC link is brought down?

At this point, the MAC link has not been brought down (i.e. netif_carrier_off
and MAC teardown haven't occurred) because phylink_link_down() is deferred to
run inside phylink_resolve().

Could disabling the PCS while the MAC is still actively transmitting/receiving
violate hardware sequencing and trigger bus stalls, dropped packets, or
hardware hangs?

> +
> +		pl->force_major_config = true;
> +		pl->pcs = NULL;
> +	}
> +
> +	mutex_unlock(&pl->state_mutex);
> +
> +	/* 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]
Will ethtool advertise stale link modes since pl->supported is not updated?

This code correctly shrinks pl->supported_interfaces to exclude the released
PCS's capabilities, but fails to correspondingly re-evaluate the global
pl->supported link mode mask.

[Severity: Medium]
Should pl->sfp_interfaces also be updated when a PCS is released?

sfp_interfaces represents the intersection between the currently inserted
SFP module's capabilities and pl->supported_interfaces. Since
pl->supported_interfaces shrinks, not updating pl->sfp_interfaces might lead
phylink to incorrectly validate or attempt to configure an SFP link interface
mode that is now impossible to satisfy with the remaining hardware.

> +
> +	phylink_run_resolve(pl);
> +}
> +EXPORT_SYMBOL_GPL(phylink_release_pcs);

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