netdev - Re: [PATCH ethtool-next] rxclass: Make output for RSS context action explicit

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: <58302551-352b-2d9e-1914-b9032942cfa3@gmail.com>
Date: Fri, 8 Nov 2024 21:13:50 +0000
From: Edward Cree <ecree.xilinx@...il.com>
To: Joe Damato <jdamato@...tly.com>, Daniel Xu <dxu@...uu.xyz>,
 davem@...emloft.net, mkubecek@...e.cz, kuba@...nel.org,
 martin.lau@...ux.dev, netdev@...r.kernel.org, kernel-team@...a.com
Subject: Re: [PATCH ethtool-next] rxclass: Make output for RSS context action
 explicit

On 08/11/2024 20:34, Joe Damato wrote:
> On Fri, Nov 08, 2024 at 07:56:41PM +0000, Edward Cree wrote:
>> I believe this patch is incorrect.  My understanding is that on
>>  packet reception, the integer returned from the RSS indirection
>>  table is *added* to the queue number from the ntuple rule, so
>>  that for instance the same indirection table can be used for one
>>  rule distributing packets over queues 0-3 and for another rule
>>  distributing a different subset of packets over queues 4-7.
>> I'm not sure if this behaviour is documented anywhere, and
>>  different NICs may have different interpretations, but this is
>>  how sfc ef10 behaves.

I've looked up the history, and my original commit[1] adding RSS +
 ntuple specified this addition behaviour in both the patch
 description and the ethtool uapi header comments.  The kerneldoc
 comment for struct ethtool_rxnfc still has this language:
 * For %ETHTOOL_SRXCLSRLINS, @fs specifies the rule to add or update.
 * @fs.@...ation either specifies the location to use or is a special
 * location value with %RX_CLS_LOC_SPECIAL flag set.  On return,
 * @fs.@...ation is the actual rule location.  If @fs.@...w_type
 * includes the %FLOW_RSS flag, @rss_context is the RSS context ID to
 * use for flow spreading traffic which matches this rule.  The value
 * from the rxfh indirection table will be added to @fs.@...g_cookie
 * to choose which ring to deliver to.
The ethtool man page, however, does not document this.

> I just wanted to chime in and say that my understanding has always
> been more aligned with Daniel's and I had also found the ethtool
> output confusing when directing flows that match a rule to a custom
> context.
> 
> If Daniel's patch is wrong (I don't know enough to say if it is or
> not), would it be possible to have some alternate ethtool output
> that's less confusing? Or for this specific output to be outlined in
> the documentation somewhere?

I think sensible output would be to keep Daniel's "Action: Direct to
 RSS context id %u", but also print something like "Queue base offset:
 %u" with the ring index that was previously printed as the Action.
If the base offset is zero its output could possibly be suppressed.
And we should update the ethtool man page to describe the adding
 behaviour, and audit device drivers to ensure that any that don't
 support it reject RSS filters with nonzero ring_cookie, as specified
 in [1].
Does this sound reasonable?

-ed

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=84a1d9c4820080bebcbd413a845076dcb62f45fa