| Message ID | 20240806160126.551306-1-edward.cree@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: ethtool: fix off-by-one error in max RSS context IDs | expand |
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")
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 */
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/
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.
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;