[PATCH net v2] bnxt_en: Bound SW TPA IDs to prevent crashes

[PATCH net v2] bnxt_en: Bound SW TPA IDs to prevent crashes

[Joe Damato]

TPA IDs are generated by FW and can be up to 1024. bnxt_alloc_agg_idx is
intended to wrap the FW ID to a value in the range of [0, 255] and
generate a mapping between FW IDs and the wrapped software ID.

On a 57608 with firmware version 233, the firmware advertises 32
concurrent TPAs. As of the commit under fixes, bp->max_tpa on this NIC
is set to 32.

If the software ID from bnxt_alloc_agg_idx is above 31, this results in
an invalid address being loaded on this line:

  tpa_info = &rxr->rx_tpa[agg_id];

because rx_tpa is allocated with only bp->max_tpa (32) entries. Writes
to tpa_info later in the code are out of bounds.

This bug results in a crash at boot:

Oops: general protection fault, kernel NULL pointer dereference 0x8: 0000 [#1] SMP NOPTI
RIP: 0010:bnxt_rx_pkt+0xc0/0x1560
RSP: 0018:ffffc900009b8c78 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000048 RCX: 0000000206682516
RDX: ffffc900009b8db4 RSI: 0000000000000000 RDI: 01ffffff038fe1c0
RBP: ffffc9006e687480 R08: ffffc9006e687000 R09: 0000000000003048
R10: 0000000000000480 R11: ffff8881c6083900 R12: 0000000006682516
R13: ffff8881c6095400 R14: 0000000000000016 R15: ffff8881c6b66680
FS:  0000000000000000(0000) GS:ffff88fef3c77000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fc8bda40584 CR3: 000000807c812001 CR4: 0000000008772ef0
PKRU: 55555554
Call Trace:
 <IRQ>
 ? __netif_receive_skb_list_core+0x1ca/0x250
 __bnxt_poll_work+0x152/0x280
 bnxt_poll_p5+0x1cd/0x480
 __napi_poll+0x30/0x180
 net_rx_action+0x20b/0x3b0
 ? note_gp_changes+0x53/0xe0
 ? tick_setup_sched_timer+0x180/0x180
 ? __napi_schedule+0x9a/0xb0
 ? bnxt_msix+0x24/0x30
 handle_softirqs+0xdd/0x2c0
 __irq_exit_rcu.llvm.3171231171502365008+0x47/0xf0
 common_interrupt+0x85/0x90
 </IRQ>
 <TASK>
 asm_common_interrupt+0x22/0x40

This stack trace is from a crash triggered when an out of bounds rx_tpa
is dereferenced. The invalid write mentioned above is silent in this
particular crash.

Fix this by allocating rx_tpa with bp->max_tpa rounded up to the next
power of 2 (bp->max_tpa_roundup_size) entries and masking the FW TPA ID
with that size, so the wrapped ID can never index past the end of the
array.

Fixes: 54c28fab2fa5 ("bnxt_en: Set bp->max_tpa according to what the FW supports")
Reported-by: Raphael Cardoso Fernandes <raphaelcf@meta.com>
Suggested-by: Michael Chan <michael.chan@broadcom.com>
Cc: stable@vger.kernel.org
Signed-off-by: Joe Damato <joe@dama.to>
---
v2:
  - Followed Michael's suggestion on the v1 to increase the size of the tpa
    array so that wrapping indexes into the array is a simple mask.
  - Add Suggested-by because the approach was suggested by Micahel.
  - Add a Reported-by so that Raphael gets credit for reporting this bug.
  - Boot tested on a machine with a 57608 and the crash did not reproduce.

v1: https://lore.kernel.org/netdev/20260821233549.3134699-1-joe@dama.to/

 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 25 ++++++++++++++---------
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  2 +-
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 9c2cc50276a5..ba710704b192 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -1517,14 +1517,16 @@ static int bnxt_discard_rx(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 	return 0;
 }
 
-static u16 bnxt_alloc_agg_idx(struct bnxt_rx_ring_info *rxr, u16 agg_id)
+static u16 bnxt_alloc_agg_idx(struct bnxt *bp, struct bnxt_rx_ring_info *rxr,
+			      u16 agg_id)
 {
 	struct bnxt_tpa_idx_map *map = rxr->rx_tpa_idx_map;
-	u16 idx = agg_id & MAX_TPA_P5_MASK;
+	u16 idx = agg_id & (bp->max_tpa_roundup_size - 1);
 
 	if (test_bit(idx, map->agg_idx_bmap)) {
-		idx = find_first_zero_bit(map->agg_idx_bmap, MAX_TPA_P5);
-		if (idx >= MAX_TPA_P5)
+		idx = find_first_zero_bit(map->agg_idx_bmap,
+					  bp->max_tpa_roundup_size);
+		if (idx >= bp->max_tpa_roundup_size)
 			return INVALID_HW_RING_ID;
 	}
 	__set_bit(idx, map->agg_idx_bmap);
@@ -1589,7 +1591,7 @@ static void bnxt_tpa_start(struct bnxt *bp, struct bnxt_rx_ring_info *rxr,
 
 	if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
 		agg_id = TPA_START_AGG_ID_P5(tpa_start);
-		agg_id = bnxt_alloc_agg_idx(rxr, agg_id);
+		agg_id = bnxt_alloc_agg_idx(bp, rxr, agg_id);
 		if (unlikely(agg_id == INVALID_HW_RING_ID)) {
 			netdev_warn(bp->dev, "Unable to allocate agg ID for ring %d, agg 0x%x\n",
 				    rxr->bnapi->index,
@@ -3587,7 +3589,7 @@ static void bnxt_free_one_tpa_info_data(struct bnxt *bp,
 {
 	int i;
 
-	for (i = 0; i < bp->max_tpa; i++) {
+	for (i = 0; i < bp->max_tpa_roundup_size; i++) {
 		struct bnxt_tpa_info *tpa_info = &rxr->rx_tpa[i];
 		u8 *data = tpa_info->data;
 
@@ -3784,7 +3786,7 @@ static void bnxt_free_one_tpa_info(struct bnxt *bp,
 	kfree(rxr->rx_tpa_idx_map);
 	rxr->rx_tpa_idx_map = NULL;
 	if (rxr->rx_tpa) {
-		for (i = 0; i < bp->max_tpa; i++) {
+		for (i = 0; i < bp->max_tpa_roundup_size; i++) {
 			kfree(rxr->rx_tpa[i].agg_arr);
 			rxr->rx_tpa[i].agg_arr = NULL;
 		}
@@ -3810,13 +3812,14 @@ static int bnxt_alloc_one_tpa_info(struct bnxt *bp,
 	struct rx_agg_cmp *agg;
 	int i;
 
-	rxr->rx_tpa = kzalloc_objs(struct bnxt_tpa_info, bp->max_tpa);
+	rxr->rx_tpa = kzalloc_objs(struct bnxt_tpa_info,
+				   bp->max_tpa_roundup_size);
 	if (!rxr->rx_tpa)
 		return -ENOMEM;
 
 	if (!(bp->flags & BNXT_FLAG_CHIP_P5_PLUS))
 		return 0;
-	for (i = 0; i < bp->max_tpa; i++) {
+	for (i = 0; i < bp->max_tpa_roundup_size; i++) {
 		agg = kzalloc_objs(*agg, MAX_SKB_FRAGS);
 		if (!agg)
 			return -ENOMEM;
@@ -3843,6 +3846,8 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
 			bp->max_tpa = MAX_TPA_P5;
 	}
 
+	bp->max_tpa_roundup_size = roundup_pow_of_two(bp->max_tpa);
+
 	for (i = 0; i < bp->rx_nr_rings; i++) {
 		struct bnxt_rx_ring_info *rxr = &bp->rx_ring[i];
 
@@ -4554,7 +4559,7 @@ static int bnxt_alloc_one_tpa_info_data(struct bnxt *bp,
 	u8 *data;
 	int i;
 
-	for (i = 0; i < bp->max_tpa; i++) {
+	for (i = 0; i < bp->max_tpa_roundup_size; i++) {
 		data = __bnxt_alloc_rx_frag(bp, &mapping, rxr,
 					    GFP_KERNEL);
 		if (!data)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index ab894f8addef..de46b42d7c98 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -789,7 +789,6 @@ struct nqe_cn {
 
 #define MAX_TPA		64
 #define MAX_TPA_P5	256
-#define MAX_TPA_P5_MASK	(MAX_TPA_P5 - 1)
 #define MAX_TPA_SEGS_P5	0x3f
 
 #if (BNXT_PAGE_SHIFT == 16)
@@ -2380,6 +2379,7 @@ struct bnxt {
 
 	u16			max_tpa_v2;
 	u16			max_tpa;
+	u16			max_tpa_roundup_size;
 	u32			rx_buf_size;
 	u32			rx_buf_use_size;	/* useable size */
 	u16			rx_offset;
-- 
2.53.0-Meta

Re: [PATCH net v2] bnxt_en: Bound SW TPA IDs to prevent crashes

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

On Mon, Aug 24, 2026 at 5:18 PM Joe Damato <joe@dama.to> wrote:
> Fix this by allocating rx_tpa with bp->max_tpa rounded up to the next
> power of 2 (bp->max_tpa_roundup_size) entries and masking the FW TPA ID
> with that size, so the wrapped ID can never index past the end of the
> array.
>
> Fixes: 54c28fab2fa5 ("bnxt_en: Set bp->max_tpa according to what the FW supports")
> Reported-by: Raphael Cardoso Fernandes <raphaelcf@meta.com>
> Suggested-by: Michael Chan <michael.chan@broadcom.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Joe Damato <joe@dama.to>

Thanks.
Reviewed-by: Michael Chan <michael.chan@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]

Re: [PATCH net v2] bnxt_en: Bound SW TPA IDs to prevent crashes

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
bnxt_en: Bound SW TPA IDs to prevent crashes

The patch allocates rxr->rx_tpa with bp->max_tpa rounded up to the next
power of two, stored in a new bp->max_tpa_roundup_size field, and masks the
firmware TPA ID with that size in bnxt_alloc_agg_idx() instead of using
MAX_TPA_P5_MASK.  All of the TPA info allocation and free loops now use the
new field as their bound.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 9c2cc50276a59..ba710704b1921 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -1517,14 +1517,16 @@ static int bnxt_discard_rx(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
>  	return 0;
>  }
>  
> -static u16 bnxt_alloc_agg_idx(struct bnxt_rx_ring_info *rxr, u16 agg_id)
> +static u16 bnxt_alloc_agg_idx(struct bnxt *bp, struct bnxt_rx_ring_info *rxr,
> +			      u16 agg_id)
>  {
>  	struct bnxt_tpa_idx_map *map = rxr->rx_tpa_idx_map;
> -	u16 idx = agg_id & MAX_TPA_P5_MASK;
> +	u16 idx = agg_id & (bp->max_tpa_roundup_size - 1);
>  
>  	if (test_bit(idx, map->agg_idx_bmap)) {
> -		idx = find_first_zero_bit(map->agg_idx_bmap, MAX_TPA_P5);
> -		if (idx >= MAX_TPA_P5)
> +		idx = find_first_zero_bit(map->agg_idx_bmap,
> +					  bp->max_tpa_roundup_size);
> +		if (idx >= bp->max_tpa_roundup_size)
>  			return INVALID_HW_RING_ID;
>  	}
>  	__set_bit(idx, map->agg_idx_bmap);

[Severity: Medium]

This isn't a bug introduced by this patch, but the value stored is now
bounded while the index it is stored at is still the raw firmware ID, just
below the last quoted line:

	map->agg_id_tbl[agg_id] = idx;

agg_id comes from TPA_START_AGG_ID_P5(), whose descriptor field is 12 bits
wide:

	#define RX_TPA_START_CMP_AGG_ID_P5	(0x0fff << 16)

and the table it indexes is 1024 entries, immediately followed by the
in-use bitmap:

struct bnxt_tpa_idx_map {
	u16		agg_id_tbl[1024];
	DECLARE_BITMAP(agg_idx_bmap, MAX_TPA_P5);
};

Can an ID of 1024 or above write past agg_id_tbl[] into agg_idx_bmap?

The same raw ID is used for the read side in bnxt_lookup_agg_idx(), whose
result is used unchecked by bnxt_tpa_end() and bnxt_tpa_agg():

	agg_id = bnxt_lookup_agg_idx(rxr, agg_id);
	tpa_info = &rxr->rx_tpa[agg_id];

The commit message says "TPA IDs are generated by FW and can be up to
1024", which matches the table size, and I could not find a capability
field that would allow firmware to emit an ID above 1023, so this may not
be reachable in practice.

Would an agg_id &= ARRAY_SIZE(map->agg_id_tbl) - 1 in both helpers be worth
adding while this function is being touched?

[ ... ]

> @@ -3784,7 +3786,7 @@ static void bnxt_free_one_tpa_info(struct bnxt *bp,
>  	kfree(rxr->rx_tpa_idx_map);
>  	rxr->rx_tpa_idx_map = NULL;
>  	if (rxr->rx_tpa) {
> -		for (i = 0; i < bp->max_tpa; i++) {
> +		for (i = 0; i < bp->max_tpa_roundup_size; i++) {
>  			kfree(rxr->rx_tpa[i].agg_arr);
>  			rxr->rx_tpa[i].agg_arr = NULL;
>  		}
> @@ -3810,13 +3812,14 @@ static int bnxt_alloc_one_tpa_info(struct bnxt *bp,
>  	struct rx_agg_cmp *agg;
>  	int i;
>  
> -	rxr->rx_tpa = kzalloc_objs(struct bnxt_tpa_info, bp->max_tpa);
> +	rxr->rx_tpa = kzalloc_objs(struct bnxt_tpa_info,
> +				   bp->max_tpa_roundup_size);
>  	if (!rxr->rx_tpa)
>  		return -ENOMEM;
>  
>  	if (!(bp->flags & BNXT_FLAG_CHIP_P5_PLUS))
>  		return 0;
> -	for (i = 0; i < bp->max_tpa; i++) {
> +	for (i = 0; i < bp->max_tpa_roundup_size; i++) {
>  		agg = kzalloc_objs(*agg, MAX_SKB_FRAGS);
>  		if (!agg)
>  			return -ENOMEM;

[Severity: High]

This is a pre-existing issue, but this loop now runs up to
bp->max_tpa_roundup_size times (for example 33 becoming 64), so there are
more chances for the 272 byte kzalloc_objs(*agg, MAX_SKB_FRAGS) to fail.

On failure, bnxt_alloc_one_tpa_info() returns before rx_tpa_idx_map is
replaced, and the caller for a queue restart goes to the cleanup path:

	if (bp->flags & BNXT_FLAG_TPA) {
		rc = bnxt_alloc_one_tpa_info(bp, clone);
		if (rc)
			goto err_free_tpa_info;
	...
err_free_tpa_info:
	bnxt_free_one_tpa_info(bp, clone);

clone is a shallow copy of the live ring made in bnxt_queue_mem_alloc():

	memcpy(clone, rxr, sizeof(*rxr));
	bnxt_init_rx_ring_struct(bp, clone);
	bnxt_reset_rx_ring_struct(bp, clone);
	...
	clone->rx_agg_bmap = NULL;

Only rx_agg_bmap is cleared there, and bnxt_reset_rx_ring_struct() does not
touch rx_tpa or rx_tpa_idx_map.

So does bnxt_free_one_tpa_info(bp, clone) free the running ring's map here?

	kfree(rxr->rx_tpa_idx_map);
	rxr->rx_tpa_idx_map = NULL;

The live ring's own rx_tpa_idx_map field is not cleared by that, and the
NAPI path writes through it on every TPA completion, in
bnxt_alloc_agg_idx():

	__set_bit(idx, map->agg_idx_bmap);
	map->agg_id_tbl[agg_id] = idx;

with a second kfree() of the same pointer when the source ring is torn down.
Should clone->rx_tpa and clone->rx_tpa_idx_map be set to NULL alongside
clone->rx_agg_bmap before allocating?

> @@ -3843,6 +3846,8 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
>  			bp->max_tpa = MAX_TPA_P5;
>  	}
>  
> +	bp->max_tpa_roundup_size = roundup_pow_of_two(bp->max_tpa);
> +
>  	for (i = 0; i < bp->rx_nr_rings; i++) {
>  		struct bnxt_rx_ring_info *rxr = &bp->rx_ring[i];
>  

[Severity: Medium]

Can the early return earlier in bnxt_alloc_tpa_info() skip this assignment
while bp->max_tpa is already set?

	bp->max_tpa = MAX_TPA;
	if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
		if (!bp->max_tpa_v2)
			return 0;

On a P5+ device whose firmware does not report max_aggs_supported,
bp->max_tpa_v2 is 0, so bp->max_tpa_roundup_size keeps its zero value while
bp->max_tpa is MAX_TPA.

bnxt_alloc_one_tpa_info() is also reachable from bnxt_queue_mem_alloc(),
which is gated only on the TPA flag and not on max_tpa_v2:

	if (bp->flags & BNXT_FLAG_TPA) {
		rc = bnxt_alloc_one_tpa_info(bp, clone);

With a count of 0, does kzalloc_objs(struct bnxt_tpa_info, 0) return
ZERO_SIZE_PTR and pass the following check?

	if (!rxr->rx_tpa)
		return -ENOMEM;

And in bnxt_alloc_agg_idx(), does the new mask become a no-op when the field
is 0?

	u16 idx = agg_id & (bp->max_tpa_roundup_size - 1);

That would leave idx as large as 4095 for the test_bit()/__set_bit() calls
on the 256 bit agg_idx_bmap and for &rxr->rx_tpa[idx] in bnxt_tpa_start().

BNXT_FLAG_TPA can be set in that state because bnxt_set_rx_skb_mode(), which
bnxt_xdp_set() calls on XDP detach, adds NETIF_F_LRO to hw_features without
a BNXT_SUPPORTS_TPA() check:

		if (rx > 1) {
			bp->flags &= ~BNXT_FLAG_NO_AGG_RINGS;
			bp->dev->hw_features |= NETIF_F_LRO;
		}

Would setting bp->max_tpa_roundup_size next to every bp->max_tpa assignment
keep the two in sync?

> @@ -4554,7 +4559,7 @@ static int bnxt_alloc_one_tpa_info_data(struct bnxt *bp,
>  	u8 *data;
>  	int i;
>  
> -	for (i = 0; i < bp->max_tpa; i++) {
> +	for (i = 0; i < bp->max_tpa_roundup_size; i++) {
>  		data = __bnxt_alloc_rx_frag(bp, &mapping, rxr,
>  					    GFP_KERNEL);
>  		if (!data)

[Severity: High]

This is a pre-existing issue, but this loop now performs up to twice as many
GFP_KERNEL allocations, and the queue restart caller ignores the -ENOMEM
that follows the quoted check, in bnxt_queue_mem_alloc():

	if (bp->flags & BNXT_FLAG_TPA)
		bnxt_alloc_one_tpa_info_data(bp, clone);

	return 0;

The open path checks the same call, so is the missing check here
intentional?

If the first __bnxt_alloc_rx_frag() fails, the remaining rxr->rx_tpa[i]
entries keep data == NULL and mapping == 0, and bnxt_queue_start() installs
the array into the live ring:

	rxr->rx_tpa = clone->rx_tpa;
	rxr->rx_tpa_idx_map = clone->rx_tpa_idx_map;

Does bnxt_tpa_start() then post a zero DMA address to the device and a NULL
buffer into the software ring for such an entry?

	prod_rx_buf->data = tpa_info->data;
	prod_rx_buf->data_ptr = tpa_info->data_ptr;

	mapping = tpa_info->mapping;
	prod_rx_buf->mapping = mapping;
	...
	prod_bd->rx_bd_haddr = cpu_to_le64(mapping);

When that descriptor completes, bnxt_rx_pkt() uses rx_buf->data without a
NULL check.
-- 
This is an AI-generated review.

Re: [PATCH net v2] bnxt_en: Bound SW TPA IDs to prevent crashes

[Joe Damato]

On Thu, Aug 27, 2026 at 10:31:39AM +0200, Paolo Abeni wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> bnxt_en: Bound SW TPA IDs to prevent crashes
> 
> The patch allocates rxr->rx_tpa with bp->max_tpa rounded up to the next
> power of two, stored in a new bp->max_tpa_roundup_size field, and masks the
> firmware TPA ID with that size in bnxt_alloc_agg_idx() instead of using
> MAX_TPA_P5_MASK.  All of the TPA info allocation and free loops now use the
> new field as their bound.
> 
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index 9c2cc50276a59..ba710704b1921 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -1517,14 +1517,16 @@ static int bnxt_discard_rx(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
> >  	return 0;
> >  }
> >  
> > -static u16 bnxt_alloc_agg_idx(struct bnxt_rx_ring_info *rxr, u16 agg_id)
> > +static u16 bnxt_alloc_agg_idx(struct bnxt *bp, struct bnxt_rx_ring_info *rxr,
> > +			      u16 agg_id)
> >  {
> >  	struct bnxt_tpa_idx_map *map = rxr->rx_tpa_idx_map;
> > -	u16 idx = agg_id & MAX_TPA_P5_MASK;
> > +	u16 idx = agg_id & (bp->max_tpa_roundup_size - 1);
> >  
> >  	if (test_bit(idx, map->agg_idx_bmap)) {
> > -		idx = find_first_zero_bit(map->agg_idx_bmap, MAX_TPA_P5);
> > -		if (idx >= MAX_TPA_P5)
> > +		idx = find_first_zero_bit(map->agg_idx_bmap,
> > +					  bp->max_tpa_roundup_size);
> > +		if (idx >= bp->max_tpa_roundup_size)
> >  			return INVALID_HW_RING_ID;
> >  	}
> >  	__set_bit(idx, map->agg_idx_bmap);
> 
> [Severity: Medium]
> 
> This isn't a bug introduced by this patch, but the value stored is now
> bounded while the index it is stored at is still the raw firmware ID, just
> below the last quoted line:
> 
> 	map->agg_id_tbl[agg_id] = idx;
> 
> agg_id comes from TPA_START_AGG_ID_P5(), whose descriptor field is 12 bits
> wide:
> 
> 	#define RX_TPA_START_CMP_AGG_ID_P5	(0x0fff << 16)
> 
> and the table it indexes is 1024 entries, immediately followed by the
> in-use bitmap:
> 
> struct bnxt_tpa_idx_map {
> 	u16		agg_id_tbl[1024];
> 	DECLARE_BITMAP(agg_idx_bmap, MAX_TPA_P5);
> };
> 
> Can an ID of 1024 or above write past agg_id_tbl[] into agg_idx_bmap?

I don't think this is possible. bp->max_tpa is min_t(u16, bp->max_tpa_v2,
MAX_TPA_P5), never going above 256. 1024 is 4x the headroom and if FW returned
that, it would be ignoring the count it advertised... which seems like a FW
bug.

> The same raw ID is used for the read side in bnxt_lookup_agg_idx(), whose
> result is used unchecked by bnxt_tpa_end() and bnxt_tpa_agg():
> 
> 	agg_id = bnxt_lookup_agg_idx(rxr, agg_id);
> 	tpa_info = &rxr->rx_tpa[agg_id];
> 
> The commit message says "TPA IDs are generated by FW and can be up to
> 1024", which matches the table size, and I could not find a capability
> field that would allow firmware to emit an ID above 1023, so this may not
> be reachable in practice.

Agreed, it is not reachable.

> 
> > @@ -3784,7 +3786,7 @@ static void bnxt_free_one_tpa_info(struct bnxt *bp,
> >  	kfree(rxr->rx_tpa_idx_map);
> >  	rxr->rx_tpa_idx_map = NULL;
> >  	if (rxr->rx_tpa) {
> > -		for (i = 0; i < bp->max_tpa; i++) {
> > +		for (i = 0; i < bp->max_tpa_roundup_size; i++) {
> >  			kfree(rxr->rx_tpa[i].agg_arr);
> >  			rxr->rx_tpa[i].agg_arr = NULL;
> >  		}
> > @@ -3810,13 +3812,14 @@ static int bnxt_alloc_one_tpa_info(struct bnxt *bp,
> >  	struct rx_agg_cmp *agg;
> >  	int i;
> >  
> > -	rxr->rx_tpa = kzalloc_objs(struct bnxt_tpa_info, bp->max_tpa);
> > +	rxr->rx_tpa = kzalloc_objs(struct bnxt_tpa_info,
> > +				   bp->max_tpa_roundup_size);
> >  	if (!rxr->rx_tpa)
> >  		return -ENOMEM;
> >  
> >  	if (!(bp->flags & BNXT_FLAG_CHIP_P5_PLUS))
> >  		return 0;
> > -	for (i = 0; i < bp->max_tpa; i++) {
> > +	for (i = 0; i < bp->max_tpa_roundup_size; i++) {
> >  		agg = kzalloc_objs(*agg, MAX_SKB_FRAGS);
> >  		if (!agg)
> >  			return -ENOMEM;
> 
> [Severity: High]
> 
> This is a pre-existing issue, but this loop now runs up to
> bp->max_tpa_roundup_size times (for example 33 becoming 64), so there are
> more chances for the 272 byte kzalloc_objs(*agg, MAX_SKB_FRAGS) to fail.
> 
> On failure, bnxt_alloc_one_tpa_info() returns before rx_tpa_idx_map is
> replaced, and the caller for a queue restart goes to the cleanup path:
> 
> 	if (bp->flags & BNXT_FLAG_TPA) {
> 		rc = bnxt_alloc_one_tpa_info(bp, clone);
> 		if (rc)
> 			goto err_free_tpa_info;
> 	...
> err_free_tpa_info:
> 	bnxt_free_one_tpa_info(bp, clone);
> 
> clone is a shallow copy of the live ring made in bnxt_queue_mem_alloc():
> 
> 	memcpy(clone, rxr, sizeof(*rxr));
> 	bnxt_init_rx_ring_struct(bp, clone);
> 	bnxt_reset_rx_ring_struct(bp, clone);
> 	...
> 	clone->rx_agg_bmap = NULL;
> 
> Only rx_agg_bmap is cleared there, and bnxt_reset_rx_ring_struct() does not
> touch rx_tpa or rx_tpa_idx_map.
> 
> So does bnxt_free_one_tpa_info(bp, clone) free the running ring's map here?
> 
> 	kfree(rxr->rx_tpa_idx_map);
> 	rxr->rx_tpa_idx_map = NULL;
> 
> The live ring's own rx_tpa_idx_map field is not cleared by that, and the
> NAPI path writes through it on every TPA completion, in
> bnxt_alloc_agg_idx():
> 
> 	__set_bit(idx, map->agg_idx_bmap);
> 	map->agg_id_tbl[agg_id] = idx;
> 
> with a second kfree() of the same pointer when the source ring is torn down.
> Should clone->rx_tpa and clone->rx_tpa_idx_map be set to NULL alongside
> clone->rx_agg_bmap before allocating?

I think this is a real bug, but as mentioned above this is a pre-existing
issue and not related to the patch I posted.

I can send a separate patch in the future addressing this, if desired, but I
don't think it makes sense to try to address this in this patch since it would
need a different fixes.
 
> > @@ -3843,6 +3846,8 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
> >  			bp->max_tpa = MAX_TPA_P5;
> >  	}
> >  
> > +	bp->max_tpa_roundup_size = roundup_pow_of_two(bp->max_tpa);
> > +
> >  	for (i = 0; i < bp->rx_nr_rings; i++) {
> >  		struct bnxt_rx_ring_info *rxr = &bp->rx_ring[i];
> >  
> 
> [Severity: Medium]
> 
> Can the early return earlier in bnxt_alloc_tpa_info() skip this assignment
> while bp->max_tpa is already set?
> 
> 	bp->max_tpa = MAX_TPA;
> 	if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
> 		if (!bp->max_tpa_v2)
> 			return 0;
> 
> On a P5+ device whose firmware does not report max_aggs_supported,
> bp->max_tpa_v2 is 0, so bp->max_tpa_roundup_size keeps its zero value while
> bp->max_tpa is MAX_TPA.
> 
> bnxt_alloc_one_tpa_info() is also reachable from bnxt_queue_mem_alloc(),
> which is gated only on the TPA flag and not on max_tpa_v2:
> 
> 	if (bp->flags & BNXT_FLAG_TPA) {
> 		rc = bnxt_alloc_one_tpa_info(bp, clone);
> 
> With a count of 0, does kzalloc_objs(struct bnxt_tpa_info, 0) return
> ZERO_SIZE_PTR and pass the following check?
> 
> 	if (!rxr->rx_tpa)
> 		return -ENOMEM;
> 
> And in bnxt_alloc_agg_idx(), does the new mask become a no-op when the field
> is 0?
> 
> 	u16 idx = agg_id & (bp->max_tpa_roundup_size - 1);
> 
> That would leave idx as large as 4095 for the test_bit()/__set_bit() calls
> on the 256 bit agg_idx_bmap and for &rxr->rx_tpa[idx] in bnxt_tpa_start().
> 
> BNXT_FLAG_TPA can be set in that state because bnxt_set_rx_skb_mode(), which
> bnxt_xdp_set() calls on XDP detach, adds NETIF_F_LRO to hw_features without
> a BNXT_SUPPORTS_TPA() check:
> 
> 		if (rx > 1) {
> 			bp->flags &= ~BNXT_FLAG_NO_AGG_RINGS;
> 			bp->dev->hw_features |= NETIF_F_LRO;
> 		}
> 
> Would setting bp->max_tpa_roundup_size next to every bp->max_tpa assignment
> keep the two in sync?

OK, this is real and I will fix this in the v3.

> > @@ -4554,7 +4559,7 @@ static int bnxt_alloc_one_tpa_info_data(struct bnxt *bp,
> >  	u8 *data;
> >  	int i;
> >  
> > -	for (i = 0; i < bp->max_tpa; i++) {
> > +	for (i = 0; i < bp->max_tpa_roundup_size; i++) {
> >  		data = __bnxt_alloc_rx_frag(bp, &mapping, rxr,
> >  					    GFP_KERNEL);
> >  		if (!data)
> 
> [Severity: High]
> 
> This is a pre-existing issue, but this loop now performs up to twice as many
> GFP_KERNEL allocations, and the queue restart caller ignores the -ENOMEM
> that follows the quoted check, in bnxt_queue_mem_alloc():
> 
> 	if (bp->flags & BNXT_FLAG_TPA)
> 		bnxt_alloc_one_tpa_info_data(bp, clone);
> 
> 	return 0;
> 
> The open path checks the same call, so is the missing check here
> intentional?
> 
> If the first __bnxt_alloc_rx_frag() fails, the remaining rxr->rx_tpa[i]
> entries keep data == NULL and mapping == 0, and bnxt_queue_start() installs
> the array into the live ring:
> 
> 	rxr->rx_tpa = clone->rx_tpa;
> 	rxr->rx_tpa_idx_map = clone->rx_tpa_idx_map;
> 
> Does bnxt_tpa_start() then post a zero DMA address to the device and a NULL
> buffer into the software ring for such an entry?
> 
> 	prod_rx_buf->data = tpa_info->data;
> 	prod_rx_buf->data_ptr = tpa_info->data_ptr;
> 
> 	mapping = tpa_info->mapping;
> 	prod_rx_buf->mapping = mapping;
> 	...
> 	prod_bd->rx_bd_haddr = cpu_to_le64(mapping);
> 
> When that descriptor completes, bnxt_rx_pkt() uses rx_buf->data without a
> NULL check.

This seems like it is real, but also not related to this patch and this would
needs its own fixes, so I don't think it is worth wrapping that into the
proposed bugfix.