Re: [PATCH net-next v15 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:
- [High] phylink_validate_mac_and_pcs() fails to reject interfaces when a required PCS is not found, leading to invalid link configurations being accepted.
- [High] phylink_create() improperly unions MAC and PCS supported interfaces, leaking unsupported generic PCS modes to the phylink configuration.
--

commit 29d359b25a130406a2e1a1837648f9bffccf1373
Author: Christian Marangi <ansuelsmth@gmail.com>

    net: phylink: introduce internal phylink PCS handling
    
    Introduce internal handling of PCS for phylink. This is an alternative
    way to .mac_select_pcs that moves the selection logic of the PCS entirely
    to phylink with the usage of the supported_interface value in the PCS
    struct.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 7f7a7328124f2..d7391a60f01f6 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: High]
If a MAC declares pcs_interfaces for a mode, but no PCS in the list matches
that interface, pcs remains NULL. Does this code incorrectly proceed to 
validate the MAC capabilities without returning an error (such as -EINVAL)
when a required PCS is missing?

[ ... ]
> @@ -1909,16 +2028,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: High]
Does this unintentionally leak unsupported generic PCS modes to the phylink
configuration?

By ORing the PCS interfaces into pl->supported_interfaces, the phylink
instance may falsely advertise support for modes that the PCS supports but
the MAC hardware does not natively support, bypassing the MAC capability
restrictions. Should this restrict to the intersection instead?

>  	linkmode_fill(pl->supported);
>  	linkmode_copy(pl->link_config.advertising, pl->supported);
>  	phylink_validate(pl, pl->supported, &pl->link_config);

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