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

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: <20240806095426.6c4bcd2a@kernel.org>
Date: Tue, 6 Aug 2024 09:54:26 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: <edward.cree@....com>
Cc: <davem@...emloft.net>, <edumazet@...gle.com>, <pabeni@...hat.com>,
 Edward Cree <ecree.xilinx@...il.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net: ethtool: fix off-by-one error in max RSS
 context IDs

On Tue, 6 Aug 2024 17:01:26 +0100 edward.cree@....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@...nel.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@...nel.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