[PATCH net] net: ethtool: fix off-by-one error in max RSS context IDs

[PATCH net] net: ethtool: fix off-by-one error in max RSS context IDs

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Both ethtool_ops.rxfh_max_context_id and the default value used when
 it's not specified are supposed to be exclusive maxima (the former
 is documented as such; the latter, U32_MAX, cannot be used as an ID
 since it equals ETH_RXFH_CONTEXT_ALLOC), but xa_alloc() expects an
 inclusive maximum.
Subtract one from 'limit' to produce an inclusive maximum, and pass
 that to xa_alloc().  Special-case limit==0 to avoid overflow.

Fixes: 6603754cd914 ("net: ethtool: let the core choose RSS context IDs")
Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 net/ethtool/ioctl.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 8ca13208d240..de34ea9b9665 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1453,8 +1453,13 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 			u32 ctx_id;
 
 			/* driver uses new API, core allocates ID */
-			ret = xa_alloc(&dev->ethtool->rss_ctx, &ctx_id, ctx,
-				       XA_LIMIT(1, limit), GFP_KERNEL_ACCOUNT);
+			if (limit)
+				ret = xa_alloc(&dev->ethtool->rss_ctx, &ctx_id,
+					       ctx, XA_LIMIT(1, limit - 1),
+					       GFP_KERNEL_ACCOUNT);
+			else
+				/* match xa_alloc's 'no free entries' result */
+				ret = -EBUSY;
 			if (ret < 0) {
 				kfree(ctx);
 				goto out;

Re: [PATCH net] net: ethtool: fix off-by-one error in max RSS context IDs

[Edward Cree]

On 06/08/2024 17:01, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Both ethtool_ops.rxfh_max_context_id and the default value used when
>  it's not specified are supposed to be exclusive maxima (the former
>  is documented as such; the latter, U32_MAX, cannot be used as an ID
>  since it equals ETH_RXFH_CONTEXT_ALLOC), but xa_alloc() expects an
>  inclusive maximum.
> Subtract one from 'limit' to produce an inclusive maximum, and pass
>  that to xa_alloc().  Special-case limit==0 to avoid overflow.
> 
> Fixes: 6603754cd914 ("net: ethtool: let the core choose RSS context IDs")

Whoops, that should have been
Fixes: 847a8ab18676 ("net: ethtool: let the core choose RSS context IDs")

Re: [PATCH net] net: ethtool: fix off-by-one error in max RSS context IDs

[Jakub Kicinski]

On Tue, 6 Aug 2024 17:01:26 +0100 edward.cree@amd.com wrote:
> Subtract one from 'limit' to produce an inclusive maximum, and pass
>  that to xa_alloc().  Special-case limit==0 to avoid overflow.

It can't be zero

	u32 limit = ops->rxfh_max_context_id ?: U32_MAX - 1;

also1 if we want to switch to exclusive I maintain we should rename the
field
also2 check that it's not 1 during registration, that'd be nonsense
also3 you're breaking bnxt, it _wants_ 32 to be a valid ID, with max 32

For a reference this is what I typed in in my tree yesterday:

-->8----

From 12f932531ef138dde108656074ff6175424a912f Mon Sep 17 00:00:00 2001
From: Jakub Kicinski <kuba@kernel.org>
Date: Mon, 5 Aug 2024 14:39:55 -0700
Subject: net: ethtool: fix ambiguity around rxfh_max_context_id

kdoc about @rxfh_max_context_id is fairly clear that the value
is exclusive, but code feeds it into XA_LIMIT(), which treats
it as inclusive:

 * struct xa_limit - Represents a range of IDs.
 * @min: The lowest ID to allocate (inclusive).
 * @max: The maximum ID to allocate (inclusive).

The default value also appears to expect xa_limit() to be exclusive,
as U32_MAX would conflict with:

 #define ETH_RXFH_CONTEXT_ALLOC		0xffffffff

The name of @rxfh_max_context_id indicates it's inclusive
(exclusive name should probably be something along the lines
of @rxfh_max_context_cnt). Keep the name but make sure we treat
it as inclusive.

Fixes: 847a8ab18676 ("net: ethtool: let the core choose RSS context IDs")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/ethtool.h | 4 ++--
 net/ethtool/ioctl.c     | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 8c89dc33d51c..d4df5ac67d65 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -740,9 +740,9 @@ struct kernel_ethtool_ts_info {
  * @rxfh_key_space: same as @rxfh_indir_space, but for the key.
  * @rxfh_priv_size: size of the driver private data area the core should
  *	allocate for an RSS context (in &struct ethtool_rxfh_context).
- * @rxfh_max_context_id: maximum (exclusive) supported RSS context ID.  If this
+ * @rxfh_max_context_id: maximum (inclusive) supported RSS context ID.  If this
  *	is zero then the core may choose any (nonzero) ID, otherwise the core
- *	will only use IDs strictly less than this value, as the @rss_context
+ *	will only use IDs less or equal to this value, as the @rss_context
  *	argument to @create_rxfh_context and friends.
  * @supported_coalesce_params: supported types of interrupt coalescing.
  * @supported_ring_params: supported ring params.
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index e32b791f8d1c..35b7e75c2202 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1468,7 +1468,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		}
 
 		if (ops->create_rxfh_context) {
-			u32 limit = ops->rxfh_max_context_id ?: U32_MAX;
+			u32 limit = ops->rxfh_max_context_id ?: U32_MAX - 1;
 			u32 ctx_id;
 
 			/* driver uses new API, core allocates ID */
-- 
2.45.2

Re: [PATCH net] net: ethtool: fix off-by-one error in max RSS context IDs

[Edward Cree]

On 8/6/24 17:54, Jakub Kicinski wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Tue, 6 Aug 2024 17:01:26 +0100 edward.cree@amd.com wrote:
>> Subtract one from 'limit' to produce an inclusive maximum, and pass
>>  that to xa_alloc().  Special-case limit==0 to avoid overflow.
> 
> It can't be zero
> 
>         u32 limit = ops->rxfh_max_context_id ?: U32_MAX - 1;

You're right, -ENOCAFFEINE.

> also1 if we want to switch to exclusive I maintain we should rename the
> field

Okay, will do.  I misunderstood your "if we change the definition
 of the field" remark, because in my head I'm not changing the
 definition ;)
How about rxfh_max_num_contexts?

> also2 check that it's not 1 during registration, that'd be nonsense

Fair enough.

> also3 you're breaking bnxt, it _wants_ 32 to be a valid ID, with max 32

Fwiw the limit in bnxt existed purely for the sake of the bitmap[1]
 which you removed when you converted it to the new API.
My reading of the bnxt code is that context allocation happens via
 a firmware RPC.  Pavan, if the firmware can be trusted to reject
 this RPC when it has no contexts left to give, then you shouldn't
 need an rxfh_max_context_id in the driver at all and you can
 remove it from ethtool_ops for net-next.
To avoid a regression in net I'll change it to 33 in my patch.

(Typically rxfh_max_context_id is only needed if either driver or
 firmware is using the context ID as an index into a fixed-size
 array.  This is why I consider an exclusive limit -- which would
 be set to an ARRAY_SIZE() -- more appropriate.)

-ed

[1]: https://lore.kernel.org/netdev/CALs4sv2dyy3uy+Xznm41M3uOkv1TSoGMwVBL5Cwzv=_E=+L_4A@mail.gmail.com/

Re: [PATCH net] net: ethtool: fix off-by-one error in max RSS context IDs

[Jakub Kicinski]

On Wed, 7 Aug 2024 11:30:54 +0100 Edward Cree wrote:
> > also1 if we want to switch to exclusive I maintain we should rename the
> > field  
> 
> Okay, will do.  I misunderstood your "if we change the definition
>  of the field" remark, because in my head I'm not changing the
>  definition ;)
> How about rxfh_max_num_contexts?

As good as anything I can come up with :)
It's hard to name it as "this is just for width of the contexts"
without implying that it's inclusive :S
I was thinking about hinting at this being the limit fed into XArray,
but Xarray's limit is inclusive.
Another thought I had was FIELD_MAX(), again, inclusive :D

Maybe we can forgo the max as it could imply max value, and insert id
instead because we're talking about ids not contexts?

rxfh_context_id_cnt ? Or give up and rxfh_field1_read_the_doc ;)

> > also2 check that it's not 1 during registration, that'd be nonsense  
> 
> Fair enough.
> 
> > also3 you're breaking bnxt, it _wants_ 32 to be a valid ID, with max 32  
> 
> Fwiw the limit in bnxt existed purely for the sake of the bitmap[1]
>  which you removed when you converted it to the new API.
> My reading of the bnxt code is that context allocation happens via
>  a firmware RPC.  Pavan, if the firmware can be trusted to reject
>  this RPC when it has no contexts left to give, then you shouldn't
>  need an rxfh_max_context_id in the driver at all and you can
>  remove it from ethtool_ops for net-next.
> To avoid a regression in net I'll change it to 33 in my patch.
> 
> (Typically rxfh_max_context_id is only needed if either driver or
>  firmware is using the context ID as an index into a fixed-size
>  array.  This is why I consider an exclusive limit -- which would
>  be set to an ARRAY_SIZE() -- more appropriate.)

Got it. I had the vague plan of piggy backing on this to express
the order of magnitude of how many contexts are supported, but FWIW
I no longer think it's a good fit, anyway.