netdev - Re: [PATCH RFC net-next 09/13] net: phylink: simplify fixed-link case for ksettings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 20 Jul 2020 10:52:56 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Russell King <rmk+kernel@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>
CC:     Vladimir Oltean <vladimir.oltean@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        "michael@...le.cc" <michael@...le.cc>,
        "olteanv@...il.com" <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC net-next 09/13] net: phylink: simplify fixed-link case
 for ksettings_set method

On 6/30/20 5:29 PM, Russell King wrote:
> For fixed links, we only allow the current settings, so this should be
> a matter of merely rejecting an attempt to change the settings.  If the
> settings agree, then there is nothing more we need to do.
> 
> Signed-off-by: Russell King <rmk+kernel@...linux.org.uk>

Reviewed-by: Ioana Ciornei <ioana.ciornei@....com>

> ---
>   drivers/net/phy/phylink.c | 31 ++++++++++++++++++++-----------
>   1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 967c068d16c8..b91151062cdc 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1360,22 +1360,31 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
>   		if (!s)
>   			return -EINVAL;
>   
> -		/* If we have a fixed link (as specified by firmware), refuse
> -		 * to change link parameters.
> +		/* If we have a fixed link, refuse to change link parameters.
> +		 * If the link parameters match, accept them but do nothing.
>   		 */
> -		if (pl->cur_link_an_mode == MLO_AN_FIXED &&
> -		    (s->speed != pl->link_config.speed ||
> -		     s->duplex != pl->link_config.duplex))
> -			return -EINVAL;
> +		if (pl->cur_link_an_mode == MLO_AN_FIXED) {
> +			if (s->speed != pl->link_config.speed ||
> +			    s->duplex != pl->link_config.duplex)
> +				return -EINVAL;
> +			return 0;
> +		}
>   
>   		config.speed = s->speed;
>   		config.duplex = s->duplex;
>   		break;
>   
>   	case AUTONEG_ENABLE:
> -		/* If we have a fixed link, refuse to enable autonegotiation */
> -		if (pl->cur_link_an_mode == MLO_AN_FIXED)
> -			return -EINVAL;
> +		/* If we have a fixed link, allow autonegotiation (since that
> +		 * is our default case) but do not allow the advertisement to
> +		 * be changed. If the advertisement matches, simply return.
> +		 */
> +		if (pl->cur_link_an_mode == MLO_AN_FIXED) {
> +			if (!linkmode_equal(config.advertising,
> +					    pl->link_config.advertising))
> +				return -EINVAL;
> +			return 0;
> +		}
>   
>   		config.speed = SPEED_UNKNOWN;
>   		config.duplex = DUPLEX_UNKNOWN;
> @@ -1385,8 +1394,8 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
>   		return -EINVAL;
>   	}
>   
> -	/* For a fixed link, this isn't able to change any parameters,
> -	 * which just leaves inband mode.
> +	/* We have ruled out the case with a PHY attached, and the
> +	 * fixed-link cases.  All that is left are in-band links.
>   	 */
>   	if (phylink_validate(pl, support, &config))
>   		return -EINVAL;
>