netdev - Re: [PATCH net-next] net: ethtool: Fix symmetric-xor RSS RX flow hash check

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]

Message-ID: <1f9c2f85-6922-c346-1b55-dd4666dc0977@ti.com>
Date: Wed, 27 Dec 2023 14:46:32 +0530
From: Ravi Gunasekaran <r-gunasekaran@...com>
To: Gerhard Engleder <gerhard@...leder-embedded.com>, <ahmed.zaki@...el.com>,
        <netdev@...r.kernel.org>
CC: <davem@...emloft.net>, <kuba@...nel.org>, <edumazet@...gle.com>,
        <pabeni@...hat.com>, <jia.guo@...el.com>, <jesse.brandeburg@...el.com>,
        <anthony.l.nguyen@...el.com>, <horms@...nel.org>,
        Ravi Gunasekaran
	<r-gunasekaran@...com>
Subject: Re: [PATCH net-next] net: ethtool: Fix symmetric-xor RSS RX flow hash
 check



On 12/27/23 2:25 AM, Gerhard Engleder wrote:
> Commit 13e59344fb9d ("net: ethtool: add support for symmetric-xor RSS hash")
> adds a check to the ethtool set_rxnfc operation, which checks the RX
> flow hash if the flag RXH_XFRM_SYM_XOR is set. This flag is introduced
> with the same commit. It calls the ethtool get_rxfh operation to get the
> RX flow hash data. If get_rxfh is not supported, then EOPNOTSUPP is
> returned.
> 
> There are driver like tsnep, macb, asp2, genet, gianfar, mtk, ... which
> support the ethtool operation set_rxnfc but not get_rxfh. This results
> in EOPNOTSUPP returned by ethtool_set_rxnfc() without actually calling
> the ethtool operation set_rxnfc. Thus, set_rxnfc got broken for all
> these drivers.
> 
> Check RX flow hash in ethtool_set_rxnfc() only if driver supports RX
> flow hash.
> 
> Fixes: 13e59344fb9d ("net: ethtool: add support for symmetric-xor RSS hash")
> Signed-off-by: Gerhard Engleder <gerhard@...leder-embedded.com>
> ---
>  net/ethtool/ioctl.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 86d47425038b..ec27897d1b24 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -973,32 +973,35 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
>  						u32 cmd, void __user *useraddr)
>  {
>  	const struct ethtool_ops *ops = dev->ethtool_ops;
> -	struct ethtool_rxfh_param rxfh = {};
>  	struct ethtool_rxnfc info;
>  	size_t info_size = sizeof(info);
>  	int rc;
>  
> -	if (!ops->set_rxnfc || !ops->get_rxfh)
> +	if (!ops->set_rxnfc)
>  		return -EOPNOTSUPP;
>  
>  	rc = ethtool_rxnfc_copy_struct(cmd, &info, &info_size, useraddr);
>  	if (rc)
>  		return rc;
>  
> -	rc = ops->get_rxfh(dev, &rxfh);
> -	if (rc)
> -		return rc;
> +	if (ops->get_rxfh) {
> +		struct ethtool_rxfh_param rxfh = {};
>  
> -	/* Sanity check: if symmetric-xor is set, then:
> -	 * 1 - no other fields besides IP src/dst and/or L4 src/dst
> -	 * 2 - If src is set, dst must also be set
> -	 */
> -	if ((rxfh.input_xfrm & RXH_XFRM_SYM_XOR) &&
> -	    ((info.data & ~(RXH_IP_SRC | RXH_IP_DST |
> -			    RXH_L4_B_0_1 | RXH_L4_B_2_3)) ||
> -	     (!!(info.data & RXH_IP_SRC) ^ !!(info.data & RXH_IP_DST)) ||
> -	     (!!(info.data & RXH_L4_B_0_1) ^ !!(info.data & RXH_L4_B_2_3))))
> -		return -EINVAL;
> +		rc = ops->get_rxfh(dev, &rxfh);
> +		if (rc)
> +			return rc;
> +
> +		/* Sanity check: if symmetric-xor is set, then:
> +		 * 1 - no other fields besides IP src/dst and/or L4 src/dst
> +		 * 2 - If src is set, dst must also be set
> +		 */
> +		if ((rxfh.input_xfrm & RXH_XFRM_SYM_XOR) &&
> +		    ((info.data & ~(RXH_IP_SRC | RXH_IP_DST |
> +				    RXH_L4_B_0_1 | RXH_L4_B_2_3)) ||
> +		     (!!(info.data & RXH_IP_SRC) ^ !!(info.data & RXH_IP_DST)) ||
> +		     (!!(info.data & RXH_L4_B_0_1) ^ !!(info.data & RXH_L4_B_2_3))))
> +			return -EINVAL;
> +	}
>  
>  	rc = ops->set_rxnfc(dev, &info);
>  	if (rc)

Reviewed-by: Ravi Gunasekaran <r-gunasekaran@...com>

-- 
Regards,
Ravi