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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] phylink_validate_mac_and_pcs() validates an interface as supported even when its required dedicated PCS is missing.
- [High] Missing list_del() in the unlink_pcs_list error path of phylink_create() leaves dangling list pointers in externally-owned PCS structures.
- [Medium] phylink_fill_available_pcs() uses list_add() instead of list_add_tail(), reversing the intended priority order of the provided PCS array.
--

--- Patch [2]: [PATCH net-next v9 02/12] net: phylink: introduce internal phylink PCS handling ---
commit 867ad4be39467be718a5df59cf2a9739b1c43531
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: net: phylink: introduce internal phylink PCS handling

Introduces an alternative to mac_select_pcs by shifting the PCS selection logic
entirely to phylink using the supported_interface value in the PCS struct.
A new pcs_list tracks available PCS instances.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 3464e3ec56997..d96941d4c5235 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -530,9 +552,24 @@ 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);
> +
> +		pcs_found = !!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)) {
> +		list_for_each_entry(pcs, &pl->pcs_list, list) {
> +			if (!phylink_validate_pcs_interface(pcs, state->interface)) {
> +				pcs_found = true;
> +				break;
> +			}
> +		}
>  	}
>  
> -	if (pcs) {
> +	if (pcs_found) {

[Severity: High]
If the MAC requires a dedicated PCS (test_bit returns true) but no available PCS
supports the requested interface, pcs_found remains false. Does this wrongly
skip the PCS validation block and proceed to MAC capabilities validation without
returning an error?

If so, it seems like phylink_major_config() will later fail when it performs
this same lookup, resulting in a delayed setup failure. Should we return -EINVAL
here if pcs_found is false but a dedicated PCS is required?

[ ... ]
> +static int phylink_fill_available_pcs(struct phylink *pl,
> +				      struct phylink_config *config)
> +{
[ ... ]
> +	for (i = 0; i < config->num_possible_pcs; i++) {
> +		struct phylink_pcs *pcs = pcss[i];
> +
> +		if (!pcs)
> +			continue;
> +
> +		list_add(&pcs->list, &pl->pcs_list);
> +	}

[Severity: Medium]
Since list_add() inserts elements at the head of the list, does this reverse the
priority order of the pcss array provided by the MAC driver?

Because lookups return the first matching PCS in the list, would using
list_add_tail() be needed to ensure the highest-priority PCS from the array is
correctly chosen?

[ ... ]
> @@ -1854,13 +1964,17 @@ struct phylink *phylink_create(struct phylink_config *config,
[ ... ]
>  	return pl;
> +
> +unlink_pcs_list:
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		pcs->phylink = NULL;
> +free_pl:
> +	kfree(pl);
> +	return ERR_PTR(ret);
>  }
>  EXPORT_SYMBOL_GPL(phylink_create);

[Severity: High]
If phylink_create() encounters an error and takes the unlink_pcs_list path, does
omitting list_del(&pcs->list) leave dangling pointers in the externally-owned
phylink_pcs structures once the phylink instance is freed?

If a MAC driver defers probe or retries initialization, passing these same
structures back to phylink_create() could trigger list corruption or a
use-after-free since their list nodes would still point to the freed
pl->pcs_list.

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