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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20240805151317.5c006ff7@kernel.org>
Date: Mon, 5 Aug 2024 15:13:17 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Gal Pressman <gal@...dia.com>
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 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 :)