Re: [PATCH net-next v20 07/15] net: phylink: add capabilities helper and track modified advertising

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] net: phylink: unchecked phylink_validate() in
  phylink_calculate_capabilities()
- [High] net: phylink: broken advertising tracking in
  phylink_calculate_capabilities()
--

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

net: phylink: add capabilities helper and track modified advertising

This commit introduces phylink_calculate_capabilities() and a new
advertising mask in struct phylink to recalculate supported and advertised
link modes while preserving user-disabled advertisements.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index d1bd7109c5668..46b2aba336bd1 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -61,6 +61,7 @@ struct phylink {
>  	u8 link_port;			/* The current non-phy ethtool port */
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported_lpi);
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
>  
>  	/* The link configuration settings */
>  	struct phylink_link_state link_config;
> @@ -717,6 +718,27 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
>  	return ret;
>  }
>  
> +static void phylink_calculate_capabilities(struct phylink *pl)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(disabled_advertising);
> +
> +	/* Get user-disabled advertising modes */
> +	linkmode_xor(disabled_advertising, pl->advertising,
> +		     pl->link_config.advertising);
> +
> +	/* Calculate the full supported advertisement */
> +	linkmode_fill(pl->supported);
> +	linkmode_copy(pl->link_config.advertising, pl->supported);
> +	phylink_validate(pl, pl->supported, &pl->link_config);
> +
> +	/* Save the newly calculated full advertisement */
> +	linkmode_copy(pl->advertising, pl->link_config.advertising);

[Severity: High]
What happens to pl->supported, pl->advertising, and
pl->link_config.advertising when phylink_validate() fails?

When the PCS supporting pl->link_config.interface has not attached yet or
has just been released, __phylink_validate() fails the
test_bit(state->interface, interfaces) check and returns -EINVAL before
masking supported or state->advertising.  Because the return value of
phylink_validate() is ignored, pl->supported, pl->advertising, and
pl->link_config.advertising remain filled with all ones, corrupting both
the active capabilities and the pl->advertising baseline used by later
phylink_calculate_capabilities() calls.

> +
> +	/* Keep user-disabled modes out of the active advertisement. */
> +	linkmode_andnot(pl->link_config.advertising,
> +			pl->link_config.advertising, disabled_advertising);
> +}

[Severity: High]
Does phylink_calculate_capabilities() preserve user-disabled modes and
mode-specific capabilities across PCS release and re-attach?

First, disabled_advertising is computed from pl->advertising and
pl->link_config.advertising on each call, and then pl->advertising is
overwritten with the new capabilities.  When a PCS is removed, its modes
are cleared from both masks; when it is later re-attached, linkmode_xor()
sees 0 ^ 0 = 0 for those modes and re-enables any modes the user had
disabled before removal.

Second, because linkmode_xor() is symmetric rather than
linkmode_andnot(disabled_advertising, pl->advertising,
pl->link_config.advertising), any bit present in
pl->link_config.advertising but absent from pl->advertising (such as pause
bits updated by phylink_update_pause_state()) is placed into
disabled_advertising and stripped.

Third, phylink_create() calls phylink_calculate_capabilities() before
phylink_parse_mode() and phylink_parse_fixedlink(), which (along with
phylink_set_fixed_link(), phylink_bringup_phy(), and
phylink_sfp_set_config()) overwrite pl->supported and
pl->link_config.advertising without updating pl->advertising.  A later
phylink_calculate_capabilities() call then treats the difference between
the stale initial pl->advertising and the mode-specific
pl->link_config.advertising as user-disabled modes, while
linkmode_fill(pl->supported) clobbers the mode-specific pl->supported
mask.

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