[PATCH net 0/4] ethtool: rss: minor fixes for recent RSS changes

[PATCH net 0/4] ethtool: rss: minor fixes for recent RSS changes

[Jakub Kicinski]

Make sure RSS_GET messages are consistent in do and dump.
Fix up a recently added safety check for RSS + queue offset.
Adjust related tests so that they pass on devices which
don't support RSS + queue offset.

Jakub Kicinski (4):
  ethtool: rss: fix hiding unsupported fields in dumps
  ethtool: ntuple: fix rss + ring_cookie check
  selftests: drv-net: rss_ctx: add missing cleanup in queue reconfigure
  selftests: drv-net: rss_ctx: don't fail reconfigure test if queue
    offset not supported

 net/ethtool/ioctl.c                               | 2 +-
 net/ethtool/rss.c                                 | 3 ++-
 tools/testing/selftests/drivers/net/hw/rss_ctx.py | 9 ++++++++-
 3 files changed, 11 insertions(+), 3 deletions(-)

-- 
2.48.1

[PATCH net 1/4] ethtool: rss: fix hiding unsupported fields in dumps

[Jakub Kicinski]

Commit ec6e57beaf8b ("ethtool: rss: don't report key if device
doesn't support it") intended to stop reporting key fields for
additional rss contexts if device has a global hashing key.

Later we added dump support and the filtering wasn't properly
added there. So we end up reporting the key fields in dumps
but not in dos:

  # ./pyynl/cli.py --spec netlink/specs/ethtool.yaml --do rss-get \
		--json '{"header": {"dev-index":2}, "context": 1 }'
  {
     "header": { ... },
     "context": 1,
     "indir": [0, 1, 2, 3, ...]]
  }

  # ./pyynl/cli.py --spec netlink/specs/ethtool.yaml --dump rss-get
  [
     ... snip context 0 ...
     { "header": { ... },
       "context": 1,
       "indir": [0, 1, 2, 3, ...],
 ->    "input_xfrm": 255,
 ->    "hfunc": 1,
 ->    "hkey": "000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
     }
  ]

Hide these fields correctly.

The drivers/net/hw/rss_ctx.py selftest catches this when run on
a device with single key, already:

  # Check| At /root/./ksft-net-drv/drivers/net/hw/rss_ctx.py, line 381, in test_rss_context_dump:
  # Check|     ksft_ne(set(data.get('hkey', [1])), {0}, "key is all zero")
  # Check failed {0} == {0} key is all zero
  not ok 8 rss_ctx.test_rss_context_dump

Fixes: f6122900f4e2 ("ethtool: rss: support dumping RSS contexts")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/rss.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
index 7cb106b590ab..58df9ad02ce8 100644
--- a/net/ethtool/rss.c
+++ b/net/ethtool/rss.c
@@ -107,6 +107,8 @@ rss_prepare_ctx(const struct rss_req_info *request, struct net_device *dev,
 	u32 total_size, indir_bytes;
 	u8 *rss_config;
 
+	data->no_key_fields = !dev->ethtool_ops->rxfh_per_ctx_key;
+
 	ctx = xa_load(&dev->ethtool->rss_ctx, request->rss_context);
 	if (!ctx)
 		return -ENOENT;
@@ -153,7 +155,6 @@ rss_prepare_data(const struct ethnl_req_info *req_base,
 		if (!ops->cap_rss_ctx_supported && !ops->create_rxfh_context)
 			return -EOPNOTSUPP;
 
-		data->no_key_fields = !ops->rxfh_per_ctx_key;
 		return rss_prepare_ctx(request, dev, data, info);
 	}
 
-- 
2.48.1

[PATCH net 2/4] ethtool: ntuple: fix rss + ring_cookie check

[Jakub Kicinski]

The info.flow_type is for RXFH commands, ntuple flow_type is inside
the flow spec. The check currently does nothing, as info.flow_type
is 0 for ETHTOOL_SRXCLSRLINS.

Fixes: 9e43ad7a1ede ("net: ethtool: only allow set_rxnfc with rss + ring_cookie if driver opts in")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 34bee42e1247..7609ce2b2c5e 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -993,7 +993,7 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
 		return rc;
 
 	/* Nonzero ring with RSS only makes sense if NIC adds them together */
-	if (cmd == ETHTOOL_SRXCLSRLINS && info.flow_type & FLOW_RSS &&
+	if (cmd == ETHTOOL_SRXCLSRLINS && info.fs.flow_type & FLOW_RSS &&
 	    !ops->cap_rss_rxnfc_adds &&
 	    ethtool_get_flow_spec_ring(info.fs.ring_cookie))
 		return -EINVAL;
-- 
2.48.1

[PATCH net 3/4] selftests: drv-net: rss_ctx: add missing cleanup in queue reconfigure

[Jakub Kicinski]

Commit under Fixes adds ntuple rules but never deletes them.

Fixes: 29a4bc1fe961 ("selftest: extend test_rss_context_queue_reconfigure for action addition")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/drivers/net/hw/rss_ctx.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/hw/rss_ctx.py b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
index ca8a7edff3dd..27e24e20749f 100755
--- a/tools/testing/selftests/drivers/net/hw/rss_ctx.py
+++ b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
@@ -252,6 +252,7 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
         try:
             # this targets queue 4, which doesn't exist
             ntuple2 = ethtool_create(cfg, "-N", flow)
+            defer(ethtool, f"-N {cfg.ifname} delete {ntuple2}")
         except CmdExitFailure:
             pass
         else:
@@ -260,6 +261,7 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
         ethtool(f"-X {cfg.ifname} {ctx_ref} weight 1 0 1 0")
         # ntuple rule therefore targets queues 1 and 3
         ntuple2 = ethtool_create(cfg, "-N", flow)
+        defer(ethtool, f"-N {cfg.ifname} delete {ntuple2}")
         # should replace existing filter
         ksft_eq(ntuple, ntuple2)
         _send_traffic_check(cfg, port, ctx_ref, { 'target': (1, 3),
-- 
2.48.1

[PATCH net 4/4] selftests: drv-net: rss_ctx: don't fail reconfigure test if queue offset not supported

[Jakub Kicinski]

Vast majority of drivers does not support queue offset.
Simply return if the rss context + queue ntuple fails.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/drivers/net/hw/rss_ctx.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/drivers/net/hw/rss_ctx.py b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
index 27e24e20749f..319aaa004c40 100755
--- a/tools/testing/selftests/drivers/net/hw/rss_ctx.py
+++ b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
@@ -260,7 +260,12 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
         # change the table to target queues 0 and 2
         ethtool(f"-X {cfg.ifname} {ctx_ref} weight 1 0 1 0")
         # ntuple rule therefore targets queues 1 and 3
-        ntuple2 = ethtool_create(cfg, "-N", flow)
+        try:
+            ntuple2 = ethtool_create(cfg, "-N", flow)
+        except CmdExitFailure:
+            ksft_pr("Driver does not support rss + queue offset")
+            return
+
         defer(ethtool, f"-N {cfg.ifname} delete {ntuple2}")
         # should replace existing filter
         ksft_eq(ntuple, ntuple2)
-- 
2.48.1

Re: [PATCH net 2/4] ethtool: ntuple: fix rss + ring_cookie check

[Gal Pressman]

On 01/02/2025 3:30, Jakub Kicinski wrote:
> The info.flow_type is for RXFH commands, ntuple flow_type is inside
> the flow spec. The check currently does nothing, as info.flow_type
> is 0 for ETHTOOL_SRXCLSRLINS.

Is it zero or garbage?

> 
> Fixes: 9e43ad7a1ede ("net: ethtool: only allow set_rxnfc with rss + ring_cookie if driver opts in")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Gal Pressman <gal@nvidia.com>

Re: [PATCH net 1/4] ethtool: rss: fix hiding unsupported fields in dumps

[Gal Pressman]

On 01/02/2025 3:30, Jakub Kicinski wrote:
> Commit ec6e57beaf8b ("ethtool: rss: don't report key if device
> doesn't support it") intended to stop reporting key fields for
> additional rss contexts if device has a global hashing key.
> 
> Later we added dump support and the filtering wasn't properly
> added there. So we end up reporting the key fields in dumps
> but not in dos:
> 
>   # ./pyynl/cli.py --spec netlink/specs/ethtool.yaml --do rss-get \
> 		--json '{"header": {"dev-index":2}, "context": 1 }'
>   {
>      "header": { ... },
>      "context": 1,
>      "indir": [0, 1, 2, 3, ...]]
>   }
> 
>   # ./pyynl/cli.py --spec netlink/specs/ethtool.yaml --dump rss-get
>   [
>      ... snip context 0 ...
>      { "header": { ... },
>        "context": 1,
>        "indir": [0, 1, 2, 3, ...],
>  ->    "input_xfrm": 255,
>  ->    "hfunc": 1,
>  ->    "hkey": "000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
>      }
>   ]
> 
> Hide these fields correctly.
> 
> The drivers/net/hw/rss_ctx.py selftest catches this when run on
> a device with single key, already:
> 
>   # Check| At /root/./ksft-net-drv/drivers/net/hw/rss_ctx.py, line 381, in test_rss_context_dump:
>   # Check|     ksft_ne(set(data.get('hkey', [1])), {0}, "key is all zero")
>   # Check failed {0} == {0} key is all zero
>   not ok 8 rss_ctx.test_rss_context_dump
> 
> Fixes: f6122900f4e2 ("ethtool: rss: support dumping RSS contexts")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Gal Pressman <gal@nvidia.com>

Re: [PATCH net 1/4] ethtool: rss: fix hiding unsupported fields in dumps

[Joe Damato]

On Fri, Jan 31, 2025 at 05:30:37PM -0800, Jakub Kicinski wrote:
> Commit ec6e57beaf8b ("ethtool: rss: don't report key if device
> doesn't support it") intended to stop reporting key fields for
> additional rss contexts if device has a global hashing key.
> 
> Later we added dump support and the filtering wasn't properly
> added there. So we end up reporting the key fields in dumps
> but not in dos:
> 
>   # ./pyynl/cli.py --spec netlink/specs/ethtool.yaml --do rss-get \
> 		--json '{"header": {"dev-index":2}, "context": 1 }'
>   {
>      "header": { ... },
>      "context": 1,
>      "indir": [0, 1, 2, 3, ...]]
>   }
> 
>   # ./pyynl/cli.py --spec netlink/specs/ethtool.yaml --dump rss-get
>   [
>      ... snip context 0 ...
>      { "header": { ... },
>        "context": 1,
>        "indir": [0, 1, 2, 3, ...],
>  ->    "input_xfrm": 255,
>  ->    "hfunc": 1,
>  ->    "hkey": "000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
>      }
>   ]
> 
> Hide these fields correctly.
> 
> The drivers/net/hw/rss_ctx.py selftest catches this when run on
> a device with single key, already:
> 
>   # Check| At /root/./ksft-net-drv/drivers/net/hw/rss_ctx.py, line 381, in test_rss_context_dump:
>   # Check|     ksft_ne(set(data.get('hkey', [1])), {0}, "key is all zero")
>   # Check failed {0} == {0} key is all zero
>   not ok 8 rss_ctx.test_rss_context_dump
> 
> Fixes: f6122900f4e2 ("ethtool: rss: support dumping RSS contexts")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/ethtool/rss.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

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

Re: [PATCH net 2/4] ethtool: ntuple: fix rss + ring_cookie check

[Joe Damato]

On Fri, Jan 31, 2025 at 05:30:38PM -0800, Jakub Kicinski wrote:
> The info.flow_type is for RXFH commands, ntuple flow_type is inside
> the flow spec. The check currently does nothing, as info.flow_type
> is 0 for ETHTOOL_SRXCLSRLINS.

Agree with Gal; I think ethtool's stack allocated ethtool_rxnfc
could result in some garbage value being passed in for
info.flow_type.

But I don't think it's worth respinning for such a minor nit in the
commit message.

> Fixes: 9e43ad7a1ede ("net: ethtool: only allow set_rxnfc with rss + ring_cookie if driver opts in")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/ethtool/ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH net 3/4] selftests: drv-net: rss_ctx: add missing cleanup in queue reconfigure

[Joe Damato]

On Fri, Jan 31, 2025 at 05:30:39PM -0800, Jakub Kicinski wrote:
> Commit under Fixes adds ntuple rules but never deletes them.
> 
> Fixes: 29a4bc1fe961 ("selftest: extend test_rss_context_queue_reconfigure for action addition")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/drivers/net/hw/rss_ctx.py | 2 ++
>  1 file changed, 2 insertions(+)

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

Re: [PATCH net 4/4] selftests: drv-net: rss_ctx: don't fail reconfigure test if queue offset not supported

[Joe Damato]

On Fri, Jan 31, 2025 at 05:30:40PM -0800, Jakub Kicinski wrote:
> Vast majority of drivers does not support queue offset.
> Simply return if the rss context + queue ntuple fails.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/drivers/net/hw/rss_ctx.py | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 

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

Re: [PATCH net 2/4] ethtool: ntuple: fix rss + ring_cookie check

[Jakub Kicinski]

On Mon, 3 Feb 2025 13:16:46 -0800 Joe Damato wrote:
> On Fri, Jan 31, 2025 at 05:30:38PM -0800, Jakub Kicinski wrote:
> > The info.flow_type is for RXFH commands, ntuple flow_type is inside
> > the flow spec. The check currently does nothing, as info.flow_type
> > is 0 for ETHTOOL_SRXCLSRLINS.  
> 
> Agree with Gal; I think ethtool's stack allocated ethtool_rxnfc
> could result in some garbage value being passed in for
> info.flow_type.

I admit I haven't dug into the user space side, but in the kernel
my reading is that the entire struct ethtool_rxnfc, which includes
_both_ flow_type fields gets copied in. IOW struct ethtool_rxnfc
has two flow_type fields, one directly in the struct and one inside 
the fs member.

Re: [PATCH net 2/4] ethtool: ntuple: fix rss + ring_cookie check

[Joe Damato]

On Mon, Feb 03, 2025 at 01:25:19PM -0800, Jakub Kicinski wrote:
> On Mon, 3 Feb 2025 13:16:46 -0800 Joe Damato wrote:
> > On Fri, Jan 31, 2025 at 05:30:38PM -0800, Jakub Kicinski wrote:
> > > The info.flow_type is for RXFH commands, ntuple flow_type is inside
> > > the flow spec. The check currently does nothing, as info.flow_type
> > > is 0 for ETHTOOL_SRXCLSRLINS.  
> > 
> > Agree with Gal; I think ethtool's stack allocated ethtool_rxnfc
> > could result in some garbage value being passed in for
> > info.flow_type.
> 
> I admit I haven't dug into the user space side, but in the kernel
> my reading is that the entire struct ethtool_rxnfc, which includes
> _both_ flow_type fields gets copied in. IOW struct ethtool_rxnfc
> has two flow_type fields, one directly in the struct and one inside 
> the fs member.

Agree with you there; there are two fields and I think your change
is correct. I think the nit is just the wording of the commit
message as ethtool's user space stack might have some junk where
info.flow_type is (instead of 0). I only very briefly skimmed the
ethtool side, so perhaps I missed something.

In any case: IMHO, I don't think it's worth resending just for a
minor commit message tweak.

Re: [PATCH net 2/4] ethtool: ntuple: fix rss + ring_cookie check

[Jakub Kicinski]

On Mon, 3 Feb 2025 13:30:17 -0800 Joe Damato wrote:
> > I admit I haven't dug into the user space side, but in the kernel
> > my reading is that the entire struct ethtool_rxnfc, which includes
> > _both_ flow_type fields gets copied in. IOW struct ethtool_rxnfc
> > has two flow_type fields, one directly in the struct and one inside 
> > the fs member.  
> 
> Agree with you there; there are two fields and I think your change
> is correct. I think the nit is just the wording of the commit
> message as ethtool's user space stack might have some junk where
> info.flow_type is (instead of 0). I only very briefly skimmed the
> ethtool side, so perhaps I missed something.

Oh, I thought you meant kernel doesn't init it.
Fair point, user space may pass garbage.

Re: [PATCH net 0/4] ethtool: rss: minor fixes for recent RSS changes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 31 Jan 2025 17:30:36 -0800 you wrote:
> Make sure RSS_GET messages are consistent in do and dump.
> Fix up a recently added safety check for RSS + queue offset.
> Adjust related tests so that they pass on devices which
> don't support RSS + queue offset.
> 
> Jakub Kicinski (4):
>   ethtool: rss: fix hiding unsupported fields in dumps
>   ethtool: ntuple: fix rss + ring_cookie check
>   selftests: drv-net: rss_ctx: add missing cleanup in queue reconfigure
>   selftests: drv-net: rss_ctx: don't fail reconfigure test if queue
>     offset not supported
> 
> [...]

Here is the summary with links:
  - [net,1/4] ethtool: rss: fix hiding unsupported fields in dumps
    https://git.kernel.org/netdev/net/c/244f8aa46fa9
  - [net,2/4] ethtool: ntuple: fix rss + ring_cookie check
    https://git.kernel.org/netdev/net/c/2b91cc1214b1
  - [net,3/4] selftests: drv-net: rss_ctx: add missing cleanup in queue reconfigure
    https://git.kernel.org/netdev/net/c/de379dfd9ada
  - [net,4/4] selftests: drv-net: rss_ctx: don't fail reconfigure test if queue offset not supported
    https://git.kernel.org/netdev/net/c/c3da585509ae

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net 2/4] ethtool: ntuple: fix rss + ring_cookie check

[Gal Pressman]

On 03/02/2025 23:30, Joe Damato wrote:
> On Mon, Feb 03, 2025 at 01:25:19PM -0800, Jakub Kicinski wrote:
>> On Mon, 3 Feb 2025 13:16:46 -0800 Joe Damato wrote:
>>> On Fri, Jan 31, 2025 at 05:30:38PM -0800, Jakub Kicinski wrote:
>>>> The info.flow_type is for RXFH commands, ntuple flow_type is inside
>>>> the flow spec. The check currently does nothing, as info.flow_type
>>>> is 0 for ETHTOOL_SRXCLSRLINS.  
>>>
>>> Agree with Gal; I think ethtool's stack allocated ethtool_rxnfc
>>> could result in some garbage value being passed in for
>>> info.flow_type.
>>
>> I admit I haven't dug into the user space side, but in the kernel
>> my reading is that the entire struct ethtool_rxnfc, which includes
>> _both_ flow_type fields gets copied in. IOW struct ethtool_rxnfc
>> has two flow_type fields, one directly in the struct and one inside 
>> the fs member.
> 
> Agree with you there; there are two fields and I think your change
> is correct. I think the nit is just the wording of the commit
> message as ethtool's user space stack might have some junk where
> info.flow_type is (instead of 0).

Exactly, zero vs. garbage will determine whether the check does nothing
(as written in the commit message), or have an undefined behavior.