[PATCH net] bnxt: fix head underflow on XDP head-grow

[PATCH net] bnxt: fix head underflow on XDP head-grow

[Joe Damato]

The xdp.py test test_xdp_native_adjst_head_grow_data crashes when run on
a bnxt machine (and also crashes in NIPA).

It seems that the bug is an underflow in bnxt_rx_multi_page_skb, which
builds the skb head:

  napi_build_skb(data_ptr - bp->rx_offset, rxr->rx_page_size);

The problem with this expression is that in page mode rx_offset is:

  bp->rx_offset = NET_IP_ALIGN + XDP_PACKET_HEADROOM;

Which evaluates (at least on x86_64) to 258.

The test test_xdp_native_adjst_head_grow_data tests a case where the
head is adjusted by -256.

When this test runs, data_ptr is shifted to frag_start + 2 (where
frag_start = page_address(page) + offset).

Then, bnxt_rx_multi_page_skb is invoked and the napi_build_skb
expression subtracts 258, landing at an address before frag_start. This
could be either the previous fragment or the previous physical page when
the frag_offset is < 256 (e.g. if the fragment started at offset 0).

When the skb is freed, the page pool fragment reference is dropped on
either the wrong page or the wrong frag of the right page. In either
case, the corrupted reference count can lead to the page being
prematurely recycled while still in use. Once (incorrectly) recycled, it
can be handed out again and on driver teardown this would result in a
double free.

The commit under fixes updated this code to handle the case where the
native page size is >= 64k, but it unintentionally broke the head grow
case.

To fix this, we need to do a bit of math to recover the offset if this
is a page fragment since it is not passed into rx_skb_func
(bnxt_rx_multi_page_skb, in this case).

Once the offset is recovered, build the skb at the start of the fragment
and then use skb_reserve to adjust the layout.

There are two cases, the non-adjustment case and the adjustment case.

In both cases, the skb is built at page_address(page) + frag_offset to
account for the case where the native page size >= 64K and skb_reserve
is called with data_ptr - (page_address(page) + frag_offset). That
difference equals bp->rx_offset when data_ptr was not moved, or
bp->rx_offset + xdp_adjust when XDP adjusted the head.

Re-running the failing test with this commit applied causes the test to
run successfully to completion.

The other rx_skb_func implementations don't have this issue.

Fixes: f6974b4c2d8e ("bnxt_en: Fix page pool logic for page size >= 64K")
Signed-off-by: Joe Damato <joe@dama.to>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 35e1f8f663c7..448609cc1617 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -1174,7 +1174,9 @@ static struct sk_buff *bnxt_rx_multi_page_skb(struct bnxt *bp,
 	unsigned int len = offset_and_len & 0xffff;
 	struct page *page = data;
 	u16 prod = rxr->rx_prod;
+	unsigned int frag_off;
 	struct sk_buff *skb;
+	void *frag_start;
 	int err;
 
 	err = bnxt_alloc_rx_data(bp, rxr, prod, GFP_ATOMIC);
@@ -1185,13 +1187,15 @@ static struct sk_buff *bnxt_rx_multi_page_skb(struct bnxt *bp,
 	dma_addr -= bp->rx_dma_offset;
 	dma_sync_single_for_cpu(&bp->pdev->dev, dma_addr, rxr->rx_page_size,
 				bp->rx_dir);
-	skb = napi_build_skb(data_ptr - bp->rx_offset, rxr->rx_page_size);
+	frag_off = dma_addr - page_pool_get_dma_addr(page);
+	frag_start = page_address(page) + frag_off;
+	skb = napi_build_skb(frag_start, rxr->rx_page_size);
 	if (!skb) {
 		page_pool_recycle_direct(rxr->page_pool, page);
 		return NULL;
 	}
 	skb_mark_for_recycle(skb);
-	skb_reserve(skb, bp->rx_offset);
+	skb_reserve(skb, data_ptr - (u8 *)frag_start);
 	__skb_put(skb, len);
 
 	return skb;

base-commit: 9772589b57e44aedc240211c5c3f7a684a034d3a
-- 
2.53.0-Meta

Re: [PATCH net] bnxt: fix head underflow on XDP head-grow

[Michael Chan]

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

On Mon, Jun 8, 2026 at 1:31 PM Joe Damato <joe@dama.to> wrote:
>
> The xdp.py test test_xdp_native_adjst_head_grow_data crashes when run on
> a bnxt machine (and also crashes in NIPA).
>
> It seems that the bug is an underflow in bnxt_rx_multi_page_skb, which
> builds the skb head:
>
>   napi_build_skb(data_ptr - bp->rx_offset, rxr->rx_page_size);
>
> The problem with this expression is that in page mode rx_offset is:
>
>   bp->rx_offset = NET_IP_ALIGN + XDP_PACKET_HEADROOM;
>
> Which evaluates (at least on x86_64) to 258.
>
> The test test_xdp_native_adjst_head_grow_data tests a case where the
> head is adjusted by -256.
>
> When this test runs, data_ptr is shifted to frag_start + 2 (where
> frag_start = page_address(page) + offset).
>
> Then, bnxt_rx_multi_page_skb is invoked and the napi_build_skb
> expression subtracts 258, landing at an address before frag_start. This
> could be either the previous fragment or the previous physical page when
> the frag_offset is < 256 (e.g. if the fragment started at offset 0).
>
> When the skb is freed, the page pool fragment reference is dropped on
> either the wrong page or the wrong frag of the right page. In either
> case, the corrupted reference count can lead to the page being
> prematurely recycled while still in use. Once (incorrectly) recycled, it
> can be handed out again and on driver teardown this would result in a
> double free.
>
> The commit under fixes updated this code to handle the case where the
> native page size is >= 64k, but it unintentionally broke the head grow
> case.
>
> To fix this, we need to do a bit of math to recover the offset if this
> is a page fragment since it is not passed into rx_skb_func
> (bnxt_rx_multi_page_skb, in this case).

I wonder if we should add an offset field to struct bnxt_sw_rx_bd to
simplify things for page mode.  Struct bnxt_sw_rx_agg_bd has the
offset field for a similar purpose.  Thanks.

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

Re: [PATCH net] bnxt: fix head underflow on XDP head-grow

[Joe Damato]

On Mon, Jun 08, 2026 at 09:22:07PM -0700, Michael Chan wrote:
> On Mon, Jun 8, 2026 at 1:31 PM Joe Damato <joe@dama.to> wrote:
> >
> > The xdp.py test test_xdp_native_adjst_head_grow_data crashes when run on
> > a bnxt machine (and also crashes in NIPA).
> >
> > It seems that the bug is an underflow in bnxt_rx_multi_page_skb, which
> > builds the skb head:
> >
> >   napi_build_skb(data_ptr - bp->rx_offset, rxr->rx_page_size);
> >
> > The problem with this expression is that in page mode rx_offset is:
> >
> >   bp->rx_offset = NET_IP_ALIGN + XDP_PACKET_HEADROOM;
> >
> > Which evaluates (at least on x86_64) to 258.
> >
> > The test test_xdp_native_adjst_head_grow_data tests a case where the
> > head is adjusted by -256.
> >
> > When this test runs, data_ptr is shifted to frag_start + 2 (where
> > frag_start = page_address(page) + offset).
> >
> > Then, bnxt_rx_multi_page_skb is invoked and the napi_build_skb
> > expression subtracts 258, landing at an address before frag_start. This
> > could be either the previous fragment or the previous physical page when
> > the frag_offset is < 256 (e.g. if the fragment started at offset 0).
> >
> > When the skb is freed, the page pool fragment reference is dropped on
> > either the wrong page or the wrong frag of the right page. In either
> > case, the corrupted reference count can lead to the page being
> > prematurely recycled while still in use. Once (incorrectly) recycled, it
> > can be handed out again and on driver teardown this would result in a
> > double free.
> >
> > The commit under fixes updated this code to handle the case where the
> > native page size is >= 64k, but it unintentionally broke the head grow
> > case.
> >
> > To fix this, we need to do a bit of math to recover the offset if this
> > is a page fragment since it is not passed into rx_skb_func
> > (bnxt_rx_multi_page_skb, in this case).
> 
> I wonder if we should add an offset field to struct bnxt_sw_rx_bd to
> simplify things for page mode.  Struct bnxt_sw_rx_agg_bd has the
> offset field for a similar purpose.  Thanks.

I don't mind doing that, but I wonder if that's better material for net-next?

In other words, we get the minimal fix in (this patch?) and then do the
cleanup and struct tweaking as a follow-up in net-next?

I could definitely be wrong; I had just assumed patches for net were supposed
to be as minimal as possible.

Re: [PATCH net] bnxt: fix head underflow on XDP head-grow

[Michael Chan]

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

On Tue, Jun 9, 2026 at 7:51 AM Joe Damato <joe@dama.to> wrote:
>
> On Mon, Jun 08, 2026 at 09:22:07PM -0700, Michael Chan wrote:
> > On Mon, Jun 8, 2026 at 1:31 PM Joe Damato <joe@dama.to> wrote:
> > >
> > > To fix this, we need to do a bit of math to recover the offset if this
> > > is a page fragment since it is not passed into rx_skb_func
> > > (bnxt_rx_multi_page_skb, in this case).
> >
> > I wonder if we should add an offset field to struct bnxt_sw_rx_bd to
> > simplify things for page mode.  Struct bnxt_sw_rx_agg_bd has the
> > offset field for a similar purpose.  Thanks.
>
> I don't mind doing that, but I wonder if that's better material for net-next?
>
> In other words, we get the minimal fix in (this patch?) and then do the
> cleanup and struct tweaking as a follow-up in net-next?
>
> I could definitely be wrong; I had just assumed patches for net were supposed
> to be as minimal as possible.

I think some minor refactoring/struct changes to make the fix
cleaner/simpler should be fine for -net.  I think the total number of
line changes should be about the same either way.

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

Re: [PATCH net] bnxt: fix head underflow on XDP head-grow

[Joe Damato]

On Tue, Jun 09, 2026 at 09:44:17AM -0700, Michael Chan wrote:
> On Tue, Jun 9, 2026 at 7:51 AM Joe Damato <joe@dama.to> wrote:
> >
> > On Mon, Jun 08, 2026 at 09:22:07PM -0700, Michael Chan wrote:
> > > On Mon, Jun 8, 2026 at 1:31 PM Joe Damato <joe@dama.to> wrote:
> > > >
> > > > To fix this, we need to do a bit of math to recover the offset if this
> > > > is a page fragment since it is not passed into rx_skb_func
> > > > (bnxt_rx_multi_page_skb, in this case).
> > >
> > > I wonder if we should add an offset field to struct bnxt_sw_rx_bd to
> > > simplify things for page mode.  Struct bnxt_sw_rx_agg_bd has the
> > > offset field for a similar purpose.  Thanks.
> >
> > I don't mind doing that, but I wonder if that's better material for net-next?
> >
> > In other words, we get the minimal fix in (this patch?) and then do the
> > cleanup and struct tweaking as a follow-up in net-next?
> >
> > I could definitely be wrong; I had just assumed patches for net were supposed
> > to be as minimal as possible.
> 
> I think some minor refactoring/struct changes to make the fix
> cleaner/simpler should be fine for -net.  I think the total number of
> line changes should be about the same either way.

OK. Are you OK with changing the signature of rx_skb_func? I was going to add
offset as an argument so it can be passed through.

Re: [PATCH net] bnxt: fix head underflow on XDP head-grow

[Michael Chan]

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

On Tue, Jun 9, 2026 at 10:22 AM Joe Damato <joe@dama.to> wrote:
> OK. Are you OK with changing the signature of rx_skb_func? I was going to add
> offset as an argument so it can be passed through.

We have 'rxr' and 'cons', so we should be able to get the new offset
in struct bnxt_sw_rx_bd, right?

rxr->rx_buf_ring[cons].offset

This might be simpler.

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

Re: [PATCH net] bnxt: fix head underflow on XDP head-grow

[Joe Damato]

On Tue, Jun 09, 2026 at 10:22:12AM -0700, Joe Damato wrote:
> On Tue, Jun 09, 2026 at 09:44:17AM -0700, Michael Chan wrote:
> > On Tue, Jun 9, 2026 at 7:51 AM Joe Damato <joe@dama.to> wrote:
> > >
> > > On Mon, Jun 08, 2026 at 09:22:07PM -0700, Michael Chan wrote:
> > > > On Mon, Jun 8, 2026 at 1:31 PM Joe Damato <joe@dama.to> wrote:
> > > > >
> > > > > To fix this, we need to do a bit of math to recover the offset if this
> > > > > is a page fragment since it is not passed into rx_skb_func
> > > > > (bnxt_rx_multi_page_skb, in this case).
> > > >
> > > > I wonder if we should add an offset field to struct bnxt_sw_rx_bd to
> > > > simplify things for page mode.  Struct bnxt_sw_rx_agg_bd has the
> > > > offset field for a similar purpose.  Thanks.
> > >
> > > I don't mind doing that, but I wonder if that's better material for net-next?
> > >
> > > In other words, we get the minimal fix in (this patch?) and then do the
> > > cleanup and struct tweaking as a follow-up in net-next?
> > >
> > > I could definitely be wrong; I had just assumed patches for net were supposed
> > > to be as minimal as possible.
> > 
> > I think some minor refactoring/struct changes to make the fix
> > cleaner/simpler should be fine for -net.  I think the total number of
> > line changes should be about the same either way.
> 
> OK. Are you OK with changing the signature of rx_skb_func? I was going to add
> offset as an argument so it can be passed through.

Oh, actually nevermind. I think once offset is store in the struct we can
derive like this?

frag_start = page_address(page) + rxr->rx_buf_ring[cons].offset;

And then the signature doesn't need to change.

Re: [PATCH net] bnxt: fix head underflow on XDP head-grow

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds read of NET_IP_ALIGN bytes when XDP consumes all headroom.
- [High] Leaked XDP fragments or permanent rx_agg_ring exhaustion when rx_skb_func fails.
- [High] Missing DMA Sync on Buffer Reuse corrupts device memory.
- [High] Missing XDP Metadata Handling for large packets.
- [High] Double dma_sync_single_for_cpu destroys XDP payload modifications on non-coherent architectures.
--

commit c05fa36ff7d12821126ed8ba4fc33d7ac7363af0
Author: Joe Damato <joe@dama.to>
Date:   Mon Jun 8 20:31:28 2026 +0000

    bnxt: fix head underflow on XDP head-grow
    
    The xdp.py test test_xdp_native_adjst_head_grow_data crashes when run on
    a bnxt machine (and also crashes in NIPA).
    
    It seems that the bug is an underflow in bnxt_rx_multi_page_skb, which
    builds the skb head:

[ ... ]

>     The other rx_skb_func implementations don't have this issue.

[Severity: High]
This is a pre-existing issue, but does bnxt_rx_page_skb() actually have a
similar underflow?

If an XDP program adjusts the head backwards by the full available headroom,
data_ptr is pushed to the beginning of the DMA-mapped fragment.

bnxt_rx_page_skb() then calls:

    memcpy(skb->data - NET_IP_ALIGN, data_ptr - NET_IP_ALIGN,
           payload + NET_IP_ALIGN);

Since data_ptr is at the very beginning of the memory fragment, wouldn't
subtracting NET_IP_ALIGN cause an out-of-bounds read of unmapped memory before
the page?

This same pattern appears to exist in bnxt_copy_data().

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 35e1f8f663c78..448609cc1617c 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -1174,7 +1174,9 @@ static struct sk_buff *bnxt_rx_multi_page_skb(struct bnxt *bp,
[ ... ]
>  	err = bnxt_alloc_rx_data(bp, rxr, prod, GFP_ATOMIC);
>  	if (unlikely(err)) {
>  		bnxt_reuse_rx_data(rxr, cons, data);

[Severity: High]
This isn't a bug introduced by this patch, but is device memory corrupted here
when the buffer is reused?

When XDP is active, the buffer is prepared for the CPU using
dma_sync_single_for_cpu(..., DMA_BIDIRECTIONAL).

If the allocation fails here, bnxt_reuse_rx_data() places the buffer back into
the hardware RX ring without calling dma_sync_single_for_device().

Could the CPU cache retain dirty lines that get evicted asynchronously,
corrupting the new packet data when the device DMA writes to this memory?

This also occurs in bnxt_rx_xdp() on XDP_DROP.

>  		return NULL;
>  	}
>  	dma_addr -= bp->rx_dma_offset;
>  	dma_sync_single_for_cpu(&bp->pdev->dev, dma_addr, rxr->rx_page_size,
>  				bp->rx_dir);

[Severity: High]
This is a pre-existing issue, but would this redundant sync destroy XDP payload
modifications on non-coherent architectures?

When XDP is active, bnxt_xdp_buff_init() already prepares the packet for the
CPU. The XDP program executes and may modify the packet payload in the CPU cache.

If dma_sync_single_for_cpu() is called again here unconditionally with
DMA_BIDIRECTIONAL, it might invalidate the CPU cache on non-coherent
architectures (like ARM64), causing the CPU to read stale packet data from RAM
and lose the XDP modifications.

This same pattern appears to exist in bnxt_rx_page_skb().

> -	skb = napi_build_skb(data_ptr - bp->rx_offset, rxr->rx_page_size);
> +	frag_off = dma_addr - page_pool_get_dma_addr(page);
> +	frag_start = page_address(page) + frag_off;
> +	skb = napi_build_skb(frag_start, rxr->rx_page_size);
>  	if (!skb) {
>  		page_pool_recycle_direct(rxr->page_pool, page);
>  		return NULL;
>  	}
>  	skb_mark_for_recycle(skb);
> -	skb_reserve(skb, bp->rx_offset);
> +	skb_reserve(skb, data_ptr - (u8 *)frag_start);
>  	__skb_put(skb, len);
>  
>  	return skb;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this I noticed
that XDP metadata might be dropped for large packets in bnxt_rx_pkt():

    skb = bp->rx_skb_func(bp, rxr, cons, data, data_ptr, dma_addr,
                          payload | len);

The SKB builder function (like bnxt_rx_multi_page_skb) is called without passing
the metadata length, and skb_metadata_set() is never called on this path.

Are XDP metadata silently dropped for all packets larger than rx_copybreak?


[Severity: High]
This is a pre-existing issue, but could an allocation failure here cause a leak?

In bnxt_rx_pkt():

    skb = bp->rx_skb_func(bp, rxr, cons, data, data_ptr, dma_addr,
                          payload | len);
    if (!skb)
        goto oom_next_rx;

If the SKB allocation fails (e.g., OOM), the code jumps directly to oom_next_rx,
bypassing the cleanup block for aggregated fragments (agg_bufs).

For XDP, these fragments are owned by the xdp_buff and must be freed using
bnxt_xdp_buff_frags_free(). For non-XDP, they must be skipped via
bnxt_reuse_rx_agg_bufs().

Does bypassing this cleanup lead to a permanent page pool memory leak (for XDP)
or permanent exhaustion of the hardware rx_agg_ring (for non-XDP)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608203129.920104-1-joe@dama.to?part=1