Re: [PATCH net-next v17 02/12] net: phylink: introduce internal phylink PCS handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] net: phylink: validation succeeds even if required PCS is missing
- [Medium] net: phylink: unsafe expansion of MAC supported interfaces with PCS capabilities
--

commit 3acb4b7eb5c7e2219464ad7dfac9f59c3b6954b8
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: introduce internal phylink PCS handling

This commit introduces internal handling of PCS for phylink, allowing MAC
drivers to provide a list of available PCS elements instead of implementing
the legacy mac_select_pcs callback.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a98e62fa4bd26..1156e6c61a08b 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -530,6 +549,21 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
>  		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
>  		if (IS_ERR(pcs))
>  			return PTR_ERR(pcs);
> +	/*
> +	 * Find a PCS in available PCS list for the requested interface.
> +	 *
> +	 * Skip searching if the MAC doesn't require a dedicated PCS for
> +	 * the requested interface.
> +	 */
> +	} else if (test_bit(state->interface, pl->config->pcs_interfaces)) {
> +		struct phylink_pcs *tmp;
> +
> +		list_for_each_entry(tmp, &pl->pcs_list, list) {
> +			if (!phylink_validate_pcs_interface(tmp, state->interface)) {
> +				pcs = tmp;
> +				break;
> +			}
> +		}
>  	}
>  
>  	if (pcs) {

[Severity: Medium]
When a dedicated PCS is required but none is found, does this skip the
validation block entirely since pcs remains NULL?

If the loop over pl->pcs_list finds no matching PCS, the function bypasses
the if (pcs) block but falls through to validate MAC capabilities,
ultimately returning 0 (success) instead of -EINVAL. Could this result
in phylink falsely advertising support for an interface mode the hardware
cannot use, breaking auto-negotiation fallback?

[ ... ]

> @@ -2031,16 +2150,31 @@ 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);
>  
> +	/* Fill the PCS list with available PCS from phylink config */
> +	ret = phylink_fill_available_pcs(pl, config);
> +	if (ret < 0)
> +		goto free_pl;
> +
> +	/* Link available PCS to phylink */
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		pcs->phylink = pl;
> +
>  	phy_interface_copy(pl->supported_interfaces,
>  			   pl->config->supported_interfaces);
>  
> +	/* Update 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 bitwise OR safely combine the MAC capabilities with the PCS
capabilities?

It appears to unconditionally elevate the generic PCS capabilities to the
top-level phylink instance, overwriting the MAC's strict capability
declarations. If phylink advertises support for interface modes that the
generic PCS supports but the specific MAC hardware does not, mac_prepare()
and mac_config() might be called with unrecognized modes, potentially
causing hardware configuration failures.

[ ... ]

> @@ -2076,11 +2215,20 @@ EXPORT_SYMBOL_GPL(phylink_create);
>   */
>  void phylink_destroy(struct phylink *pl)
>  {
> +	struct phylink_pcs *pcs, *tmp;
> +
>  	sfp_bus_del_upstream(pl->sfp_bus);
>  	if (pl->link_gpio)
>  		gpiod_put(pl->link_gpio);
>  
>  	cancel_work_sync(&pl->resolve);
> +
> +	/* 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);
> +	}
> +
>  	kfree(pl);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does this cleanup properly
handle the legacy mac_select_pcs users?

While this loop correctly cleans up new users populated in pl->pcs_list, it
seems to neglect clearing pl->pcs->phylink for the currently active PCS
assigned dynamically via the legacy mac_select_pcs.

If pl->pcs->phylink is left pointing to the freed pl struct for legacy MAC
drivers, could a subsequent late interrupt from the PCS trigger a
use-after-free when phylink_pcs_change() is called?

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