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: <05ae8316-d3aa-4356-98c6-55ed4253c8a7@nvidia.com>
Date: Sun, 4 Aug 2024 09:08:50 +0300
From: Gal Pressman <gal@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc: 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 03/08/2024 7:26, Jakub Kicinski wrote:
> This series is a semi-related collection of RSS patches.
> Main point is supporting dumping RSS contexts via ethtool netlink.
> At present additional RSS contexts can be queried one by one, and
> assuming user know the right IDs. This series uses the XArray
> added by Ed to provide netlink dump support for ETHTOOL_GET_RSS.
> 
> Patch 1 is a trivial selftest debug patch.
> Patch 2 coverts mvpp2 for no real reason other than that I had
> 	a grand plan of converting all drivers at some stage.
> Patch 3 removes a now moot check from mlx5 so that all tests
> 	can pass.
> Patch 4 and 5 make a bit used for context support optional,
> 	for easier grepping of drivers which need converting
> 	if nothing else.
> Patch 6 OTOH adds a new cap bit; some devices don't support
> 	using a different key per context and currently act
> 	in surprising ways.
> Patch 7 and 8 update the RSS netlink code to use XArray.
> Patch 9 and 10 add support for dumping contexts.
> Patch 11 and 12 are small adjustments to spec and a new test.

Very useful, I was messing around with the RSS code lately and was
thinking about these stuff too, thanks!

> 
> 
> I'm getting distracted with other work, so probably won't have
> the time soon to complete next steps, but things which are missing
> are (and some of these may be bad ideas):
> 
>  - better discovery
> 
>    Some sort of API to tell the user who many contexts the device
>    can create. Upper bound, devices often share contexts between
>    ports etc. so it's hard to tell exactly and upfront number of
>    contexts for a netdev. But order of magnitude (4 vs 10s) may
>    be enough for container management system to know whether to bother.
> 
>  - create/modify/delete via netlink

And actually plugging extack into set_rxfh :).

>  
>    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.