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

[Jakub Kicinski <kuba@kernel.org>]

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! We should probably split the "actual invalid" from 
the "nothing specified" checks.

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