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: <3466lphn3dex3shqg73lkya4g3pt5idvfdtyjgyhrklxwkfkkx@t4mdww7m4pwx>
Date: Fri, 8 Nov 2024 15:50:56 -0700
From: Daniel Xu <dxu@...uu.xyz>
To: Edward Cree <ecree.xilinx@...il.com>
Cc: Joe Damato <jdamato@...tly.com>, 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

Hi Ed, Joe,

Thanks for looking at this so quickly.

On Fri, Nov 08, 2024 at 09:13:50PM GMT, Edward Cree wrote:
> 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.

FWIW, bnxt on 6.9-ish is probably non-compliant (assuming this is the
correct usage of the API):

    # ethtool -N eth0 flow-type ip6 dst-ip ::1 context 1 queue 5
    Added rule with ID 0

    # ethtool -n eth0
    32 RX rings available
    Total 1 rules

    Filter: 0
            Rule Type: Raw IPv6
            Src IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff
            Dest IP addr: ::1 mask: ::
            Traffic Class: 0x0 mask: 0xff
            Protocol: 0 mask: 0xff
            L4 bytes: 0x0 mask: 0xffffffff
            RSS Context ID: 1
            Action: Direct to queue 0

Note the "Direct to queue 0" even though I specified queue 5. I'm
running a new batch of tests on 6.11 next week so I'll update here if
it magically got fixed.

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

That sounds good to me. I'll send out a v2 for the ethtool changes.

I'm probably not qualified enough to do an audit. But since I've been
poking around the bxnt driver the past week I'll give it a look and see
if I can convince myself of anything.

Thanks,
Daniel