netdev - Re: [PATCH net-next v2 00/12] ethtool: rss: driver tweaks and netlink context dumps

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 for Android: free password hash cracker in your pocket

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

Message-ID: <cbdb41f5-c157-49a5-acc0-cbce5516ea62@nvidia.com>
Date: Tue, 6 Aug 2024 15:22:07 +0300
From: Gal Pressman <gal@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
 pabeni@...hat.com, dxu@...uu.xyz, ecree.xilinx@...il.com,
 przemyslaw.kitszel@...el.com, donald.hunter@...il.com, tariqt@...dia.com,
 willemdebruijn.kernel@...il.com, jdamato@...tly.com,
 Ahmed Zaki <ahmed.zaki@...el.com>
Subject: Re: [PATCH net-next v2 00/12] ethtool: rss: driver tweaks and netlink
 context dumps

On 06/08/2024 1:13, Jakub Kicinski wrote:
> On Sun, 4 Aug 2024 09:08:50 +0300 Gal Pressman wrote:
>>>    The only question here is how to handle all the tricky IOCTL
>>>    legacy. "No change" maps trivially to attribute not present.
>>>    "reset" (indir_size = 0) probably needs to be a new NLA_FLAG?  
>>
>> FWIW, we have an incompatibility issue with the recent rxfh.input_xfrm
>> parameter.
>>
>> In ethtool_set_rxfh():
>> 	/* If either indir, hash key or function is valid, proceed further.
>> 	 * Must request at least one change: indir size, hash key, function
>> 	 * or input transformation.
>> 	 */
>> 	if ((rxfh.indir_size &&
>> 	     rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE &&
>> 	     rxfh.indir_size != dev_indir_size) ||
>> 	    (rxfh.key_size && (rxfh.key_size != dev_key_size)) ||
>> 	    (rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE &&
>> 	     rxfh.key_size == 0 && rxfh.hfunc == ETH_RSS_HASH_NO_CHANGE &&
>> 	     rxfh.input_xfrm == RXH_XFRM_NO_CHANGE))
>> 		return -EINVAL;
>>
>> When using a recent kernel with an old userspace ethtool,
>> rxfh.input_xfrm is treated as zero (which is different than
>> RXH_XFRM_NO_CHANGE) and passes the check, whereas the same command with
>> a recent userspace would result in an error.
>> This also makes it so old userspace always disables input_xfrm
>> unintentionally. I do not have any ideas on how to resolve this..
>>
>> Regardless, I believe this check is wrong as it prevents us from
>> creating RSS context with no parameters (i.e. 'ethtool -X eth0 context
>> new', as done in selftests), it works by mistake with old userspace.
>> I plan to submit a patch soon to skip this check in case of context
>> creation.
> 
> I guess we just need to throw "&& !create" into the condition?
> Sounds good! 

Yes.

> We should probably split the "actual invalid" from 
> the "nothing specified" checks.

And make the "no change" check return zero?

> 
> Also - curious what you'll put under Fixes, looks like a pretty 
> ancient bug :)

Maybe 84a1d9c48200 ("net: ethtool: extend RXNFC API to support RSS
spreading of filter matches")?