* [PATCH net v4] bnxt_en: Bound SW TPA IDs to prevent crashes
@ 2026-08-28 19:08 Joe Damato
2026-08-29 7:20 ` Michael Chan
2026-09-01 8:16 ` Paolo Abeni
0 siblings, 2 replies; 8+ messages in thread
From: Joe Damato @ 2026-08-28 19:08 UTC (permalink / raw)
To: netdev, Michael Chan, Pavan Chebbi, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, Kalesh AP,
Rukhsana Ansari
Cc: horms, colin.winegarden, linux-kernel, raphaelcf, Joe Damato,
stable, llvm
TPA IDs are generated by FW and can be up to 1024. bnxt_alloc_agg_idx is
intended to wrap the FW ID down to a software ID which is used to index
rxr->rx_tpa, and to 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>
---
v4:
- Moved bp->max_tpa_roundup_size init out of the early return path and
documented that TPA is unsupported there, as suggested by Michael.
v3: https://lore.kernel.org/netdev/20260827185700.2157164-1-joe@dama.to/
- Addressed an issue Sashiko pointed out, where max_tpa_roundup_size may be
left unset if bnxt_alloc_tpa_info returns early, which would lead to out
of bounds access.
- The other pre-existing issues Sashiko pointed out are unrelated to this
patch and would need different Fixes tags, so they are better served with
separate patches in the future.
v2: https://lore.kernel.org/netdev/20260825001842.2501798-1-joe@dama.to/
- 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 Michael.
- 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 | 28 +++++++++++++++--------
drivers/net/ethernet/broadcom/bnxt/bnxt.h | 2 +-
2 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index d59bcca73a2b..bee54cc86996 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -1514,14 +1514,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);
@@ -1586,7 +1588,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,
@@ -3584,7 +3586,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;
@@ -3781,7 +3783,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;
}
@@ -3807,13 +3809,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;
@@ -3832,6 +3835,10 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
bp->max_tpa = MAX_TPA;
if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
+ /* TPA is not supported at all, so there is nothing to
+ * allocate. BNXT_SUPPORTS_TPA() is false in this case and
+ * neither LRO nor HW GRO can be enabled.
+ */
if (!bp->max_tpa_v2)
return 0;
bp->max_tpa = min_t(u16, bp->max_tpa_v2, MAX_TPA_P5);
@@ -3839,6 +3846,7 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
if (bp->max_tpa <= 32 && BNXT_CHIP_P5(bp) && !BNXT_NPAR(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];
@@ -4551,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;
base-commit: e2a6641e3bfde58f2284f9859c2b0fdcc6d1c0da
--
2.53.0-Meta
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH net v4] bnxt_en: Bound SW TPA IDs to prevent crashes
2026-08-28 19:08 [PATCH net v4] bnxt_en: Bound SW TPA IDs to prevent crashes Joe Damato
@ 2026-08-29 7:20 ` Michael Chan
2026-09-01 17:59 ` Joe Damato
2026-09-01 8:16 ` Paolo Abeni
1 sibling, 1 reply; 8+ messages in thread
From: Michael Chan @ 2026-08-29 7:20 UTC (permalink / raw)
To: Joe Damato
Cc: netdev, Pavan Chebbi, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Nathan Chancellor, Nick Desaulniers,
Bill Wendling, Justin Stitt, Kalesh AP, Rukhsana Ansari, horms,
colin.winegarden, linux-kernel, raphaelcf, stable, llvm
[-- Attachment #1: Type: text/plain, Size: 292 bytes --]
On Fri, Aug 28, 2026 at 12:09 PM Joe Damato <joe@dama.to> wrote:
> v4:
> - Moved bp->max_tpa_roundup_size init out of the early return path and
> documented that TPA is unsupported there, as suggested by Michael.
>
Thanks.
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v4] bnxt_en: Bound SW TPA IDs to prevent crashes
2026-08-29 7:20 ` Michael Chan
@ 2026-09-01 17:59 ` Joe Damato
0 siblings, 0 replies; 8+ messages in thread
From: Joe Damato @ 2026-09-01 17:59 UTC (permalink / raw)
To: Michael Chan
Cc: netdev, Pavan Chebbi, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Nathan Chancellor, Nick Desaulniers,
Bill Wendling, Justin Stitt, Kalesh AP, Rukhsana Ansari, horms,
colin.winegarden, linux-kernel, raphaelcf, stable, llvm
On Sat, Aug 29, 2026 at 12:20:26AM -0700, Michael Chan wrote:
> On Fri, Aug 28, 2026 at 12:09 PM Joe Damato <joe@dama.to> wrote:
> > v4:
> > - Moved bp->max_tpa_roundup_size init out of the early return path and
> > documented that TPA is unsupported there, as suggested by Michael.
> >
> Thanks.
> Reviewed-by: Michael Chan <michael.chan@broadcom.com>
idk what you wanna do modulo paolo's comment on this thread.
i have a v5 locally that expands this to 5 patches which should address most
of what sashiko is upset about:
bnxt_en: Bound SW TPA IDs to prevent crashes
bnxt_en: Handle buffer allocation failure in bnxt_rx_ring_reset()
bnxt_en: Propagate TPA buffer allocation failures in bnxt_queue_mem_alloc()
bnxt_en: Don't free the live ring's TPA state on queue restart failure
bnxt_en: Only restore LRO if the device supports TPA
but ofc, who knows what other stuff sashiko is gonna find if i submit this and
how much furhter we'll push out fixing the original crash.
lmk if you wanna stick with the v4 you reviewed as-is (and i'll send the other
4 patches as a series) or if you'd rather i submit the above 5 as the v5 and
get them all in at once ?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v4] bnxt_en: Bound SW TPA IDs to prevent crashes
2026-08-28 19:08 [PATCH net v4] bnxt_en: Bound SW TPA IDs to prevent crashes Joe Damato
2026-08-29 7:20 ` Michael Chan
@ 2026-09-01 8:16 ` Paolo Abeni
2026-09-01 11:57 ` Joe Damato
1 sibling, 1 reply; 8+ messages in thread
From: Paolo Abeni @ 2026-09-01 8:16 UTC (permalink / raw)
To: Joe Damato, netdev, Michael Chan, Pavan Chebbi, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, Kalesh AP,
Rukhsana Ansari
Cc: horms, colin.winegarden, linux-kernel, raphaelcf, stable, llvm
On 8/28/26 9:08 PM, Joe Damato wrote:
> TPA IDs are generated by FW and can be up to 1024. bnxt_alloc_agg_idx is
> intended to wrap the FW ID down to a software ID which is used to index
> rxr->rx_tpa, and to 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>
> ---
> v4:
> - Moved bp->max_tpa_roundup_size init out of the early return path and
> documented that TPA is unsupported there, as suggested by Michael.
Clashiko quite convincingly elaborates that the above is not enough:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260828190900.1767611-1-joe%40dama.to
Could you please have a look?
/P
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net v4] bnxt_en: Bound SW TPA IDs to prevent crashes
2026-09-01 8:16 ` Paolo Abeni
@ 2026-09-01 11:57 ` Joe Damato
2026-09-01 12:10 ` Paolo Abeni
0 siblings, 1 reply; 8+ messages in thread
From: Joe Damato @ 2026-09-01 11:57 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev, Michael Chan, Pavan Chebbi, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Nathan Chancellor, Nick Desaulniers,
Bill Wendling, Justin Stitt, Kalesh AP, Rukhsana Ansari, horms,
colin.winegarden, linux-kernel, raphaelcf, stable, llvm
On Tue, Sep 01, 2026 at 10:16:54AM +0200, Paolo Abeni wrote:
> On 8/28/26 9:08 PM, Joe Damato wrote:
[...]
> > ---
> > v4:
> > - Moved bp->max_tpa_roundup_size init out of the early return path and
> > documented that TPA is unsupported there, as suggested by Michael.
>
> Clashiko quite convincingly elaborates that the above is not enough:
>
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260828190900.1767611-1-joe%40dama.to
>
> Could you please have a look?
I looked. There are 4 separate things Clashiko reports, so in order from top
to bottom:
1.) "This isn't a bug introduced by this patch, but the line right below the last
context line here still uses the raw firmware ID as a table index: ..."
This seems impossible. If FW emits an ID > 1024 that's a firmware bug and if
we wanted to guard against that possiblity that would be a separate patch and
a different Fixes.
2.) "This isn't a bug introduced by this patch, but the loop count here changes
from bp->max_tpa to bp->max_tpa_roundup_size, ...."
I mentioned previously in my v3 that this is real, but is pre-existing and is
a different Fixes unrelated to this change.
3.) "The new assignment sits after the max_tpa_v2 == 0 early return, so on a P5+
device whose firmware reports max_aggs_supported == 0, does
bp->max_tpa_roundup_size stay 0 while BNXT_FLAG_TPA is set? ..."
Michael commented in the v3 that this is a false positive, hence why I changed
the code from what it was in the v3 to this.
4.) "This isn't a bug introduced by this patch, but the loop bound grows here
(for example 64 instead of 48 when max_tpa is not a power of two), so there
are more mandatory allocations that can fail, and the failure is not
propagated by two callers...."
I mentioned previously in the v3 that this is real, but this is pre-existing
and unrelated to this change. Fixing this would be a separate patch with a
different Fixes.
Michael: please feel free to confirm (or let me know if I'm wrong here), but I
think this patch can be merged.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v4] bnxt_en: Bound SW TPA IDs to prevent crashes
2026-09-01 11:57 ` Joe Damato
@ 2026-09-01 12:10 ` Paolo Abeni
2026-09-01 15:03 ` Joe Damato
0 siblings, 1 reply; 8+ messages in thread
From: Paolo Abeni @ 2026-09-01 12:10 UTC (permalink / raw)
To: Joe Damato, netdev, Michael Chan, Pavan Chebbi, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, Kalesh AP,
Rukhsana Ansari, horms, colin.winegarden, linux-kernel, raphaelcf,
stable, llvm
On 9/1/26 1:57 PM, Joe Damato wrote:
> On Tue, Sep 01, 2026 at 10:16:54AM +0200, Paolo Abeni wrote:
>> On 8/28/26 9:08 PM, Joe Damato wrote:
>>> - Moved bp->max_tpa_roundup_size init out of the early return path and
>>> documented that TPA is unsupported there, as suggested by Michael.
>>
>> Clashiko quite convincingly elaborates that the above is not enough:
>>
>> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260828190900.1767611-1-joe%40dama.to
>>
>> Could you please have a look?
[...]
> 3.) "The new assignment sits after the max_tpa_v2 == 0 early return, so on a P5+
> device whose firmware reports max_aggs_supported == 0, does
> bp->max_tpa_roundup_size stay 0 while BNXT_FLAG_TPA is set? ..."
>
> Michael commented in the v3 that this is a false positive, hence why I changed
> the code from what it was in the v3 to this.
This is the comment I referred to.
AFAICS the problem is that the driver sets
bp->dev->hw_features |= NETIF_F_LRO;
regardless of the supported capabilities, and that seams to disagree
with Michael.
/P
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v4] bnxt_en: Bound SW TPA IDs to prevent crashes
2026-09-01 12:10 ` Paolo Abeni
@ 2026-09-01 15:03 ` Joe Damato
2026-09-03 9:35 ` Paolo Abeni
0 siblings, 1 reply; 8+ messages in thread
From: Joe Damato @ 2026-09-01 15:03 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev, Michael Chan, Pavan Chebbi, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Nathan Chancellor, Nick Desaulniers,
Bill Wendling, Justin Stitt, Kalesh AP, Rukhsana Ansari, horms,
colin.winegarden, linux-kernel, raphaelcf, stable, llvm
On Tue, Sep 01, 2026 at 02:10:04PM +0200, Paolo Abeni wrote:
> On 9/1/26 1:57 PM, Joe Damato wrote:
> > On Tue, Sep 01, 2026 at 10:16:54AM +0200, Paolo Abeni wrote:
> >> On 8/28/26 9:08 PM, Joe Damato wrote:
> >>> - Moved bp->max_tpa_roundup_size init out of the early return path and
> >>> documented that TPA is unsupported there, as suggested by Michael.
> >>
> >> Clashiko quite convincingly elaborates that the above is not enough:
> >>
> >> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260828190900.1767611-1-joe%40dama.to
> >>
> >> Could you please have a look?
> [...]
> > 3.) "The new assignment sits after the max_tpa_v2 == 0 early return, so on a P5+
> > device whose firmware reports max_aggs_supported == 0, does
> > bp->max_tpa_roundup_size stay 0 while BNXT_FLAG_TPA is set? ..."
> >
> > Michael commented in the v3 that this is a false positive, hence why I changed
> > the code from what it was in the v3 to this.
>
> This is the comment I referred to.
>
> AFAICS the problem is that the driver sets
>
> bp->dev->hw_features |= NETIF_F_LRO;
>
> regardless of the supported capabilities, and that seams to disagree
> with Michael.
OK, after re-reading this here's what I see: NETIF_F_LRO gets added with no
capability check and bnxt_fix_features() won't strip it afterwards. I think
the fix there is to add a BNXT_SUPPORTS_TPA() test, as a Fixes for
f0aa6a37a3db ("eth: bnxt: always recalculate features after XDP clearing, fix
null-deref").
I can send a separate patch for that, but it is unrelated to this patch which
fixes a crash on boot for Thor2 devices.
Maybe you want me to spin a v5 that rewords the comment Michael asked me to
add which is currently technically incorrect, but will be correct after the
separate bug mentioned above is fixed?
In other words respin this to make a v5 that changes the comment to:
/* TPA is not supported at all, so there is nothing to
* allocate.
*/
And then when a fixes for the above commit is proposed, the comment can be
re-expanded?
Can you let me know what I am missing here?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net v4] bnxt_en: Bound SW TPA IDs to prevent crashes
2026-09-01 15:03 ` Joe Damato
@ 2026-09-03 9:35 ` Paolo Abeni
0 siblings, 0 replies; 8+ messages in thread
From: Paolo Abeni @ 2026-09-03 9:35 UTC (permalink / raw)
To: Joe Damato, netdev, Michael Chan, Pavan Chebbi, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, Kalesh AP,
Rukhsana Ansari, horms, colin.winegarden, linux-kernel, raphaelcf,
stable, llvm
Hi,
sorry for the latency here.
On 9/1/26 5:03 PM, Joe Damato wrote:
> On Tue, Sep 01, 2026 at 02:10:04PM +0200, Paolo Abeni wrote:
>> On 9/1/26 1:57 PM, Joe Damato wrote:
>>> On Tue, Sep 01, 2026 at 10:16:54AM +0200, Paolo Abeni wrote:
>>>> On 8/28/26 9:08 PM, Joe Damato wrote:
>>>>> - Moved bp->max_tpa_roundup_size init out of the early return path and
>>>>> documented that TPA is unsupported there, as suggested by Michael.
>>>>
>>>> Clashiko quite convincingly elaborates that the above is not enough:
>>>>
>>>> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260828190900.1767611-1-joe%40dama.to
>>>>
>>>> Could you please have a look?
>> [...]
>>> 3.) "The new assignment sits after the max_tpa_v2 == 0 early return, so on a P5+
>>> device whose firmware reports max_aggs_supported == 0, does
>>> bp->max_tpa_roundup_size stay 0 while BNXT_FLAG_TPA is set? ..."
>>>
>>> Michael commented in the v3 that this is a false positive, hence why I changed
>>> the code from what it was in the v3 to this.
>>
>> This is the comment I referred to.
>>
>> AFAICS the problem is that the driver sets
>>
>> bp->dev->hw_features |= NETIF_F_LRO;
>>
>> regardless of the supported capabilities, and that seams to disagree
>> with Michael.
>
> OK, after re-reading this here's what I see: NETIF_F_LRO gets added with no
> capability check and bnxt_fix_features() won't strip it afterwards. I think
> the fix there is to add a BNXT_SUPPORTS_TPA() test, as a Fixes for
> f0aa6a37a3db ("eth: bnxt: always recalculate features after XDP clearing, fix
> null-deref").
>
> I can send a separate patch for that, but it is unrelated to this patch which
> fixes a crash on boot for Thor2 devices.
With my limited knowledge of this driver, I thought the report being
more strictly related to this patch, and the fix for them should land
together.
I'm fine with v5 approach.
/P
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-09-03 9:36 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-28 19:08 [PATCH net v4] bnxt_en: Bound SW TPA IDs to prevent crashes Joe Damato
2026-08-29 7:20 ` Michael Chan
2026-09-01 17:59 ` Joe Damato
2026-09-01 8:16 ` Paolo Abeni
2026-09-01 11:57 ` Joe Damato
2026-09-01 12:10 ` Paolo Abeni
2026-09-01 15:03 ` Joe Damato
2026-09-03 9:35 ` Paolo Abeni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox