Re: [PATCH] net: phylink: reject unsupported speed/duplex in ksettings_set() with PHY

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi,

On 7/1/26 05:17, muhammad.nazim.amirul.nazle.asmade@altera.com wrote:
> From: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>

Target tree tag is still also missing in the subject :)

> When using ethtool to change speed and duplex on a phylink-managed
> interface with a PHY attached, the requested speed/duplex combination
> is not validated against the MAC's supported capabilities before being
> passed down to the PHY layer.
> 
> commit df0acdc59b09 ("net: phylink: fix ksettings_set() ethtool call")
> and commit 03c44a21d033 ("net: phylink: actually fix ksettings_set()
> ethtool call") introduced masking of the PHY advertising modes against
> pl->supported, but did not add an explicit check that the requested
> speed/duplex itself is within the MAC's capability set.
> 
> The AUTONEG_DISABLE path in the non-PHY case already uses
> phy_caps_lookup() to validate speed/duplex against pl->supported.
> Extend the same validation to the pl->phydev path so that ethtool
> requests for unsupported speed/duplex combinations are rejected with
> -EINVAL before reaching the PHY layer.
> 
> Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
> ---
>  drivers/net/phy/phylink.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 087ac63f9193..22f9bbd381bd 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2989,6 +2989,10 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
>  	if (pl->phydev) {
>  		struct ethtool_link_ksettings phy_kset = *kset;
>  
> +		if (!phy_caps_lookup(kset->base.speed, kset->base.duplex,
> +				     pl->supported, true))
> +			return -EINVAL;
> +
>  		linkmode_and(phy_kset.link_modes.advertising,
>  			     phy_kset.link_modes.advertising,
>  			     pl->supported);

I can indeed reproduce that, with a 1000FD-only mac, running

  ethtool -s eth2 speed 1000 duplex half autoneg off

brings the link down, no error reported, and running

  ethtool -s eth2 speed 1000 duplex half autoneg on

also brings the link down, with

Advertised link modes:  Not reported

This is expected, but yeah no error reported.

I think rejecting these settings makes sense, I'm however wondering
wether this is a fix or not, as this will change user-visible behaviour.
I'd err to the side of caution and send that to net-next, but maybe
Andrew will have more insight :)

So at least, you'll have to resubmit targetting the correct tree.

Maxime