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

[Gal Pressman <gal@nvidia.com>]

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")?