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: <e6cebcb7-a3e0-076f-e099-420a143cbaaf@amd.com>
Date: Wed, 7 Aug 2024 11:30:54 +0100
From: Edward Cree <ecree@....com>
To: Jakub Kicinski <kuba@...nel.org>, Pavan Chebbi <pavan.chebbi@...adcom.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 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@....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/