[PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Gal Pressman]

Context 0 (default context) always exists, there is no need to check
whether it exists or not when adding a flow steering rule.

The existing check fails when creating a flow steering rule for context
0 as it is not stored in the rss_ctx xarray.

For example:
$ ethtool --config-ntuple eth2 flow-type tcp4 dst-ip 194.237.147.23 dst-port 19983 context 0 loc 618
rmgr: Cannot insert RX class rule: Invalid argument
Cannot insert classification rule

Fixes: de7f7582dff2 ("net: ethtool: prevent flow steering to RSS contexts which don't exist")
Cc: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
Signed-off-by: Gal Pressman <gal@nvidia.com>
---
 net/ethtool/ioctl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 98b7dcea207a..0be7a622dddf 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -998,7 +998,8 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
 		    ethtool_get_flow_spec_ring(info.fs.ring_cookie))
 			return -EINVAL;
 
-		if (!xa_load(&dev->ethtool->rss_ctx, info.rss_context))
+		if (info.rss_context &&
+		    !xa_load(&dev->ethtool->rss_ctx, info.rss_context))
 			return -EINVAL;
 	}
 
-- 
2.40.1

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Joe Damato]

On Tue, Feb 25, 2025 at 09:13:48AM +0200, Gal Pressman wrote:
> Context 0 (default context) always exists, there is no need to check
> whether it exists or not when adding a flow steering rule.
> 
> The existing check fails when creating a flow steering rule for context
> 0 as it is not stored in the rss_ctx xarray.
> 
> For example:
> $ ethtool --config-ntuple eth2 flow-type tcp4 dst-ip 194.237.147.23 dst-port 19983 context 0 loc 618
> rmgr: Cannot insert RX class rule: Invalid argument
> Cannot insert classification rule
> 
> Fixes: de7f7582dff2 ("net: ethtool: prevent flow steering to RSS contexts which don't exist")
> Cc: Jakub Kicinski <kuba@kernel.org>
> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
> Signed-off-by: Gal Pressman <gal@nvidia.com>
> ---
>  net/ethtool/ioctl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

I was initially confused why
tools/testing/selftests/drivers/net/hw/rss_ctx.py didn't catch this
bug, but if I'm reading the tests there correctly it doesn't test
this case AFAICT.

Do you think it's worth adding a test for this case as a separate
patch to net-next ?

That said, since context 0 isn't tracked in rss_ctx, the fix makes
sense to me:

Reviewed-by: Joe Damato <jdamato@fastly.com>

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Jakub Kicinski]

On Tue, 25 Feb 2025 09:13:48 +0200 Gal Pressman wrote:
> Context 0 (default context) always exists, there is no need to check
> whether it exists or not when adding a flow steering rule.
> 
> The existing check fails when creating a flow steering rule for context
> 0 as it is not stored in the rss_ctx xarray.

But what is the use case for redirecting to context 0?

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Gal Pressman]

On 26/02/2025 3:01, Jakub Kicinski wrote:
> On Tue, 25 Feb 2025 09:13:48 +0200 Gal Pressman wrote:
>> Context 0 (default context) always exists, there is no need to check
>> whether it exists or not when adding a flow steering rule.
>>
>> The existing check fails when creating a flow steering rule for context
>> 0 as it is not stored in the rss_ctx xarray.
> 
> But what is the use case for redirecting to context 0?

I can think of something like redirecting all TCP traffic to context 1,
and then a specific TCP 5-tuple to the default context.

Anyway, it used to work.

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Joe Damato]

On Tue, Feb 25, 2025 at 05:01:28PM -0800, Jakub Kicinski wrote:
> On Tue, 25 Feb 2025 09:13:48 +0200 Gal Pressman wrote:
> > Context 0 (default context) always exists, there is no need to check
> > whether it exists or not when adding a flow steering rule.
> > 
> > The existing check fails when creating a flow steering rule for context
> > 0 as it is not stored in the rss_ctx xarray.
> 
> But what is the use case for redirecting to context 0?

I think Gal's example is a good one and there could be users who are
already directing flows to context 0 and so, as Gal mentioned, this
might be a breakage? Not sure.

I'll admit that I'd typically create a custom context and then set
ntuple filters to direct traffic for those contexts (letting all the
'other' traffic land in the default context 0), so I can understand
Jakub's argument, as well.

I'm probably wrong because I'm not a python programmer, but IMHO
it'd be a nice for the python RSS tests to be updated to cover this
case (whichever way it goes). It seemed to me from a quick read
(again, likely wrong) that it wasn't covered and could lead to
confusion like this in the future?

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Jakub Kicinski]

On Wed, 26 Feb 2025 08:08:40 +0200 Gal Pressman wrote:
> On 26/02/2025 3:01, Jakub Kicinski wrote:
> > On Tue, 25 Feb 2025 09:13:48 +0200 Gal Pressman wrote:  
> >> Context 0 (default context) always exists, there is no need to check
> >> whether it exists or not when adding a flow steering rule.
> >>
> >> The existing check fails when creating a flow steering rule for context
> >> 0 as it is not stored in the rss_ctx xarray.  
> > 
> > But what is the use case for redirecting to context 0?  
> 
> I can think of something like redirecting all TCP traffic to context 1,
> and then a specific TCP 5-tuple to the default context.

The ordering guarantees of ntuple filters are a bit unclear.
My understanding was that first match terminates the search,
actually, so your example wouldn't work :S

> Anyway, it used to work.

To be clear unit tests don't count as "breaking real users",
and I assume the complaint comes from your QA team?

Given the weak definition of the ntuple API I'd prefer to
close this corner case. Unless someone feels strongly that
this should be allowed. If a real user complains we can both
fix and try to encode their flow into a selftest.

Let me CC Ed, too.

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Jakub Kicinski]

On Wed, 26 Feb 2025 18:27:17 -0800 Jakub Kicinski wrote:
> > Anyway, it used to work.  
> 
> To be clear unit tests don't count as "breaking real users",
> and I assume the complaint comes from your QA team?
> 
> Given the weak definition of the ntuple API I'd prefer to
> close this corner case. Unless someone feels strongly that
> this should be allowed. If a real user complains we can both
> fix and try to encode their flow into a selftest.
> 
> Let me CC Ed, too.

Oh, I think Ed may tell us that using context 0 + queue offset is legit.
If he does, please respin with that as the justification and the test
case as suggested by Joe.
-- 
pw-bot: cr

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Edward Cree]

On 27/02/2025 04:45, Jakub Kicinski wrote:
> The ordering guarantees of ntuple filters are a bit unclear.
> My understanding was that first match terminates the search,
> actually, so your example wouldn't work :S

My understanding is that in most ntuple implementations more-
 specific filters override less-specific ones, in which case
 Gal's setup would work.
On other implementations which use the rule number as a
 position (like the API/naming implies) you could insert the
 5-tuple rule first and that would work too.

> Oh, I think Ed may tell us that using context 0 + queue offset is legit.

I hadn't actually thought of that, but yes that's true too.

Anyway, 'mechanism, not policy' says we should allow ctx 0
 unless there's some mechanism reason why it can't be
 supported, and I don't see one.

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Jakub Kicinski]

On Thu, 27 Feb 2025 15:18:47 +0000 Edward Cree wrote:
> On 27/02/2025 04:45, Jakub Kicinski wrote:
> > The ordering guarantees of ntuple filters are a bit unclear.
> > My understanding was that first match terminates the search,
> > actually, so your example wouldn't work :S  
> 
> My understanding is that in most ntuple implementations more-
>  specific filters override less-specific ones, in which case
>  Gal's setup would work.

The behavior of partially overlapping rules being undefined?

> On other implementations which use the rule number as a
>  position (like the API/naming implies) you could insert the
>  5-tuple rule first and that would work too.
> 
> > Oh, I think Ed may tell us that using context 0 + queue offset is legit.  
> 
> I hadn't actually thought of that, but yes that's true too.
> 
> Anyway, 'mechanism, not policy' says we should allow ctx 0
>  unless there's some mechanism reason why it can't be
>  supported, and I don't see one.

I never uttered the thought that lead me to opposing. 
ctx 0 is a poor man's pass / accept. If someone needs a pass we should
add an explicit "action pass". Or am I missing something magical that
ctx 0 would do that's not 100% the same as pass (modulo the queue
offset)? Using ctx 0 as implicit pass is a very easy thing to miss
for driver developers.

But yeah, the queue offset is legit.

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Edward Cree]

On 27/02/2025 15:29, Jakub Kicinski wrote:
> On Thu, 27 Feb 2025 15:18:47 +0000 Edward Cree wrote:
>> On 27/02/2025 04:45, Jakub Kicinski wrote:
>>> The ordering guarantees of ntuple filters are a bit unclear.
>>> My understanding was that first match terminates the search,
>>> actually, so your example wouldn't work :S  
>>
>> My understanding is that in most ntuple implementations more-
>>  specific filters override less-specific ones, in which case
>>  Gal's setup would work.
> 
> The behavior of partially overlapping rules being undefined?

In the general case yes; for our implementation I think there's
 a total order of match masks defined by firmware that extends
 the partial order from the subset relation.  (Unlike our MAE
 implementation for *TC* rules on ef100, where it really is
 undefined.)

> I never uttered the thought that lead me to opposing. 
> ctx 0 is a poor man's pass / accept. If someone needs a pass we should
> add an explicit "action pass".

To me 'pass' is just a shorthand for whatever specific behaviour
 happens to match the default.  I.e. if you can already express
 that behaviour directly, then 'pass' is a strictly nonorthogonal
 addition and therefore bad interface design.
But then, I'm the guy who thinks ed(1) is a good UI, so...

> Or am I missing something magical that
> ctx 0 would do that's not 100% the same as pass (modulo the queue
> offset)?

Nothing comes to mind.

> Using ctx 0 as implicit pass is a very easy thing to miss
> for driver developers.

I don't see why drivers need anything special to handle "0 is
 pass".  They need to handle "0 isn't in the xarray" (so map it
 to the default HW context), which I suppose we ought to call out
 explicitly in the struct ethtool_rxnfc kdoc if we're going to
 continue allowing it, but it's not the fact that it's "an
 implicit pass" that makes this necessary.  If there's something
 I'm missing here please educate me :)

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Jakub Kicinski]

On Thu, 27 Feb 2025 16:24:52 +0000 Edward Cree wrote:
> > I never uttered the thought that lead me to opposing. 
> > ctx 0 is a poor man's pass / accept. If someone needs a pass we should
> > add an explicit "action pass".  
> 
> To me 'pass' is just a shorthand for whatever specific behaviour
>  happens to match the default.  I.e. if you can already express
>  that behaviour directly, then 'pass' is a strictly nonorthogonal
>  addition and therefore bad interface design.

I presume sfc matches only one rule. For devices which terminate on
first hit and rules are ordered by ntuple ID, the following rules*:

 src-ip 192.168.0.2                 context 0
 src-ip 192.0.0.0   m 0.255.255.255 action -1

implement allowing only 192.168.0.2 out of the 192.0.0.0/8 subnet. 
The device may not even support RSS contexts.

I agree that pass is not very clean, but context 0 _is_ a pass.
It's not asking for context 0, it's asking for a nop rule which
prevents a more general rule from matching. To me at least.

* coin toss on the mask polarity

> But then, I'm the guy who thinks ed(1) is a good UI, so...
> 
> > Using ctx 0 as implicit pass is a very easy thing to miss
> > for driver developers.  
> 
> I don't see why drivers need anything special to handle "0 is
>  pass".  They need to handle "0 isn't in the xarray" (so map it
>  to the default HW context), which I suppose we ought to call out
>  explicitly in the struct ethtool_rxnfc kdoc if we're going to
>  continue allowing it, but it's not the fact that it's "an
>  implicit pass" that makes this necessary.  If there's something
>  I'm missing here please educate me :)

Nothing super specific, but it does produce corner cases.
We may find out if someone got it wrong once we have the test.

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Edward Cree]

On 27/02/2025 16:56, Jakub Kicinski wrote:
> On Thu, 27 Feb 2025 16:24:52 +0000 Edward Cree wrote:
>>> I never uttered the thought that lead me to opposing. 
>>> ctx 0 is a poor man's pass / accept. If someone needs a pass we should
>>> add an explicit "action pass".  
>>
>> To me 'pass' is just a shorthand for whatever specific behaviour
>>  happens to match the default.  I.e. if you can already express
>>  that behaviour directly, then 'pass' is a strictly nonorthogonal
>>  addition and therefore bad interface design.
> 
> I presume sfc matches only one rule. For devices which terminate on
> first hit and rules are ordered by ntuple ID, the following rules*:
> 
>  src-ip 192.168.0.2                 context 0
>  src-ip 192.0.0.0   m 0.255.255.255 action -1
> 
> implement allowing only 192.168.0.2 out of the 192.0.0.0/8 subnet. 

Ah, so the point is that "pass" wasn't expressible before RSS contexts
 came along, and may therefore be surprising and new.  I get you now.

> The device may not even support RSS contexts.

I think in that case the driver will reject the first filter because it
 doesn't recognise flow_type == FLOW_RSS | WHATEVER_FLOW.  Unless the
 driver writer has deliberately added support for RSS filters despite
 only having context 0, which seems like a signal they know it means
 'pass' and explicitly wanted that.

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Gal Pressman]

On 27/02/2025 4:27, Jakub Kicinski wrote:
> On Wed, 26 Feb 2025 08:08:40 +0200 Gal Pressman wrote:
>> On 26/02/2025 3:01, Jakub Kicinski wrote:
>>> On Tue, 25 Feb 2025 09:13:48 +0200 Gal Pressman wrote:  
>>>> Context 0 (default context) always exists, there is no need to check
>>>> whether it exists or not when adding a flow steering rule.
>>>>
>>>> The existing check fails when creating a flow steering rule for context
>>>> 0 as it is not stored in the rss_ctx xarray.  
>>>
>>> But what is the use case for redirecting to context 0?  
>>
>> I can think of something like redirecting all TCP traffic to context 1,
>> and then a specific TCP 5-tuple to the default context.
> 
> The ordering guarantees of ntuple filters are a bit unclear.
> My understanding was that first match terminates the search,
> actually, so your example wouldn't work :S

The ordering should be done according to the rule location.
 * @location: Location of rule in the table.  Locations must be
 *	numbered such that a flow matching multiple rules will be
 *	classified according to the first (lowest numbered) rule.

The cited patch is a regression from user's perspective. Surely, not an
intended one?

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Jakub Kicinski]

On Sun, 2 Mar 2025 11:55:34 +0200 Gal Pressman wrote:
> >> I can think of something like redirecting all TCP traffic to context 1,
> >> and then a specific TCP 5-tuple to the default context.  
> > 
> > The ordering guarantees of ntuple filters are a bit unclear.
> > My understanding was that first match terminates the search,
> > actually, so your example wouldn't work :S  
> 
> The ordering should be done according to the rule location.
>  * @location: Location of rule in the table.  Locations must be
>  *	numbered such that a flow matching multiple rules will be
>  *	classified according to the first (lowest numbered) rule.

I'm aware, Gal, but the question is whether every driver developer 
is aware of this, not just me and perhaps you.

> The cited patch is a regression from user's perspective. Surely, not an
> intended one?

I believe this message should already answer you questions:
https://lore.kernel.org/all/20250226204503.77010912@kernel.org/
Fix the commit message, add test, repost.

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Gal Pressman]

On 04/03/2025 0:17, Jakub Kicinski wrote:
> On Sun, 2 Mar 2025 11:55:34 +0200 Gal Pressman wrote:
>>>> I can think of something like redirecting all TCP traffic to context 1,
>>>> and then a specific TCP 5-tuple to the default context.  
>>>
>>> The ordering guarantees of ntuple filters are a bit unclear.
>>> My understanding was that first match terminates the search,
>>> actually, so your example wouldn't work :S  
>>
>> The ordering should be done according to the rule location.
>>  * @location: Location of rule in the table.  Locations must be
>>  *	numbered such that a flow matching multiple rules will be
>>  *	classified according to the first (lowest numbered) rule.
> 
> I'm aware, Gal, but the question is whether every driver developer 
> is aware of this, not just me and perhaps you.
> 
>> The cited patch is a regression from user's perspective. Surely, not an
>> intended one?
> 
> I believe this message should already answer you questions:
> https://lore.kernel.org/all/20250226204503.77010912@kernel.org/
> Fix the commit message, add test, repost.

This is a bug fix targeted for net; the test should be submitted as
net-next material

Just to confirm, are you making test addition a prerequisite to merge a
bug fix?

Re: [PATCH net-next] net: ethtool: Don't check if RSS context exists in case of context 0

[Gal Pressman]

On 04/03/2025 13:09, Gal Pressman wrote:
> This is a bug fix targeted for net; the test should be submitted as
> net-next material

* Not accurate, targeted to net-next as the bug is still not in net.