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] [day] [month] [year] [list]

Message-ID: <20240807070758.07752f14@kernel.org>
Date: Wed, 7 Aug 2024 07:07:58 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Edward Cree <ecree@....com>
Cc: Pavan Chebbi <pavan.chebbi@...adcom.com>, 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 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.