netdev - Re: [PATCH net-next 09/11] eth: bnxt: bump the entry size in indir tables to u32

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: <0a790e16-792b-448c-abaa-a4bf8cc9ebb0@intel.com>
Date: Wed, 3 Jul 2024 12:51:58 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <netdev@...r.kernel.org>, <edumazet@...gle.com>, <davem@...emloft.net>,
	<pabeni@...hat.com>, <ecree.xilinx@...il.com>, <michael.chan@...adcom.com>
Subject: Re: [PATCH net-next 09/11] eth: bnxt: bump the entry size in indir
 tables to u32

On 7/3/24 01:47, Jakub Kicinski wrote:
> Ethtool core stores indirection table with u32 entries, "just to be safe".
> Switch the type in the driver, so that it's easier to swap local tables
> for the core ones.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>   drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 8 ++++----
>   drivers/net/ethernet/broadcom/bnxt/bnxt.h         | 4 ++--
>   drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 6 +++---
>   3 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 6b8966d3ecb6..4176459921b5 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -6225,7 +6225,7 @@ static u16 bnxt_cp_ring_for_tx(struct bnxt *bp, struct bnxt_tx_ring_info *txr)
>   int bnxt_alloc_rss_indir_tbl(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx)
>   {
>   	int entries;
> -	u16 *tbl;
> +	u32 *tbl;
>   
>   	if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS)
>   		entries = BNXT_MAX_RSS_TABLE_ENTRIES_P5;

it's a shame that default git context is so small, a few lines below we
have tbl alloc:
   tbl = kmalloc_array(entries, sizeof(*bp->rss_indir_tbl), GFP_KERNEL);

> @@ -6248,7 +6248,7 @@ int bnxt_alloc_rss_indir_tbl(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx)
>   void bnxt_set_dflt_rss_indir_tbl(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx)
>   {
>   	u16 max_rings, max_entries, pad, i;
> -	u16 *rss_indir_tbl;
> +	u32 *rss_indir_tbl;
>   
>   	if (!bp->rx_nr_rings)
>   		return;
> @@ -6269,12 +6269,12 @@ void bnxt_set_dflt_rss_indir_tbl(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx)
>   
>   	pad = bp->rss_indir_tbl_entries - max_entries;
>   	if (pad)
> -		memset(&rss_indir_tbl[i], 0, pad * sizeof(u16));
> +		memset(&rss_indir_tbl[i], 0, pad * sizeof(u32));
>   }

without the above allocation line (perhaps you could mention necessary 
bit of that in commit msg in case of v2) this patch could not be proved 
correct :)

with due diligence done:
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>

OTOH, I assume we need this in-driver table only to keep it over the
up→down→up cycle. Could we just keep it as inactive in the core?
(And xa_mark() it as inactive to avoid reporting to the user or any
other actions that we want to avoid)

PS. patch2 is duplicated for me