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

[sashiko-bot@kernel.org]

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