[PATCH net 0/5] eth: bnxt: fix Rx buffer leaks under OOM

[PATCH net 0/5] eth: bnxt: fix Rx buffer leaks under OOM

[Jakub Kicinski]

Commit bd3beb5c8691 ("eth: bnxt: disable rx-copybreak by default")
changed the defaults putting users on buggy(ier) driver paths.

Fix various ways bnxt may leak buffers under OOM. The first patch
fixes the real bug that was hit in production. Second patch fixes
an issue I hit while testing the first one. The next 3 patches
fix other Rx buffer leaks pointed out by an LLM during review.

I have a few more fixes for issues surfaced during review
(misplaced DMA syncs and bad event handling). I'll post those
separately.

Jakub Kicinski (5):
  eth: bnxt: recycle agg buffers when Rx head buffer allocation fails
  eth: bnxt: resize Rx head buffers when rx-copybreak changes
  eth: bnxt: don't leak buffers when XDP agg buffer allocation fails
  eth: bnxt: free XDP frags when xdp_do_redirect() fails
  eth: bnxt: hand XDP frags over to the copybreak skb

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 58 +++++++++++++------
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c |  1 +
 3 files changed, 41 insertions(+), 19 deletions(-)

-- 
2.56.0

[PATCH net 1/5] eth: bnxt: recycle agg buffers when Rx head buffer allocation fails

[Jakub Kicinski]

bnxt_rx_pkt() tries to return the aggregation buffers of a packet
to the ring when the copybreak allocation fails, but the same logic
is missing on the rx_skb_func() failure path. The agg BDs consumed
by the device are then never posted again. The agg ring is only
refilled one-for-one as completions consume its buffers, so each
such failure permanently shrinks it. Under sustained memory pressure
the ring drains completely, and the queue stops receiving anything
that does not fit into the head buffer, even once memory is available
again. Only re-creating the rings (e.g. ifdown / ifup) recovers it.

Commit 296d5b541639 ("bnxt_en: Fix aggregation buffer leak under OOM
condition.") fixed the copybreak path only. Recently
commit bd3beb5c8691 ("eth: bnxt: disable rx-copybreak by default")
made the non-copybreak path the default, re-surfacing the issue.

With XDP, the agg buffers are attached to the xdp_buff as frags by
this point. Their agg ring slots have already been refilled.
We need to keep the original head page (and therefore shinfo)
around until the xdp_buff cleanup.

Note the odd split of ownership here: rx_skb_func() only deals with
the head buffer, the agg buffers stay with the caller in whatever
state they are in, still on the ring without XDP, or frags of the
xdp_buff with XDP. This complicates rx_skb_func() cleaning up all
the buffers (head and frags), which would be more intuitive.

Discovered in production, severe packet loss would persist after
a transient OOM event. Tested on a BCM57504 by failing the head
buffer allocations in bnxt_rx_skb() (bpftrace override() of
should_failslab() and should_fail_alloc_page()) while receiving
1400B ICMPv6 echo requests. 341k packets were dropped on that queue,
enough to drain its 8188 entry agg ring 40 times over, and the queue
kept receiving large frames afterwards with no loss and no new HW drops.
With a frags-capable XDP_PASS program at MTU 9000, receiving 8000B
ICMPv6 echo requests while napi_build_skb() called from
bnxt_rx_multi_page_skb() failed, 9.2k packets were dropped,
the page pools' inflight count stayed flat and
there were no KASAN or DEBUG_PAGEALLOC splats.

Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
Fixes: 4c6c123c9af9 ("bnxt: adding bnxt_rx_agg_pages_xdp for aggregated xdp")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 33 ++++++++++++++---------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 0682060b048c..2e7276801fdd 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -1215,7 +1215,15 @@ static struct sk_buff *bnxt_rx_multi_page_skb(struct bnxt *bp,
 				bp->rx_dir);
 	skb = napi_build_skb(frag_start, rxr->rx_page_size);
 	if (!skb) {
-		page_pool_recycle_direct(rxr->page_pool, page);
+		struct page *new_page;
+
+		new_page = rxr->rx_buf_ring[RING_RX(bp, prod)].data;
+		/* The caller may still need the XDP frags from shinfo at the
+		 * end of the head page. Release the new buffer instead and
+		 * put the head back on the ring.
+		 */
+		page_pool_recycle_direct(rxr->page_pool, new_page);
+		bnxt_reuse_rx_data(rxr, cons, data);
 		return NULL;
 	}
 	skb_mark_for_recycle(skb);
@@ -2313,17 +2321,8 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 			skb = bnxt_copy_xdp(bnapi, &bnxt_xdp.xdp, len,
 					    dma_addr);
 		bnxt_reuse_rx_data(rxr, cons, data);
-		if (!skb) {
-			if (agg_bufs) {
-				if (!xdp_active)
-					bnxt_reuse_rx_agg_bufs(cpr, cp_cons, 0,
-							       agg_bufs, false);
-				else
-					bnxt_xdp_buff_frags_free(rxr,
-								 &bnxt_xdp.xdp);
-			}
-			goto oom_next_rx;
-		}
+		if (!skb)
+			goto oom_next_rx_agg;
 	} else {
 		u32 payload;
 
@@ -2334,7 +2333,7 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 		skb = bp->rx_skb_func(bp, rxr, cons, data, data_ptr, dma_addr,
 				      payload | len);
 		if (!skb)
-			goto oom_next_rx;
+			goto oom_next_rx_agg;
 	}
 
 	if (agg_bufs) {
@@ -2424,6 +2423,14 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 
 	return rc;
 
+oom_next_rx_agg:
+	if (agg_bufs) {
+		if (!xdp_active)
+			bnxt_reuse_rx_agg_bufs(cpr, cp_cons, 0, agg_bufs,
+					       false);
+		else
+			bnxt_xdp_buff_frags_free(rxr, &bnxt_xdp.xdp);
+	}
 oom_next_rx:
 	cpr->sw_stats->rx.rx_oom_discards += 1;
 	rc = -ENOMEM;
-- 
2.56.0

[PATCH net 2/5] eth: bnxt: resize Rx head buffers when rx-copybreak changes

[Jakub Kicinski]

Since rx-copybreak became configurable, bnxt_set_ring_params() sizes
the head buffers to fit at least rx_copybreak bytes, and the copybreak
path in bnxt_copy_data() syncs rx_copybreak bytes of the head buffer.
bnxt_set_tunable() however only updates bp->rx_copybreak, and nothing
on the open path recalculates the ring parameters. After raising
rx-copybreak the head buffers keep their old size until some other
setting changes, and bnxt_copy_data() syncs past their end, into the
next buffer in the page or past the end of the page:

  DMA-API: bnxt_en 0000:01:00.0: device driver tries to sync DMA memory it has not allocated [device address=0x00000002aa210c40] [size=1024 bytes]
  WARNING: kernel/dma/debug.c:1162 at check_sync+0x577/0x1eb0, CPU#22: swapper/22/0
  Call Trace:
   <IRQ>
   debug_dma_sync_single_for_cpu+0x66/0x90
   bnxt_copy_data+0x12f/0x290
   bnxt_rx_pkt+0x24ea/0x50f0
   __bnxt_poll_work+0x42c/0xc00
   bnxt_poll_p5+0x579/0x17a0
   __napi_poll+0x8b/0x2c0
   net_rx_action+0x58f/0xbb0

Here the head buffers were still sized for the 640B HDS threshold,
four 1kB buffers per page, while rx-copybreak was raised to 1024.
Without coherent DMA or with swiotlb the oversized sync can corrupt
a neighboring buffer owned by the device or by an skb.

The device must be down for rx-copybreak to change, so recalculate the
ring parameters right away, like bnxt_set_ringparam() does.

Hit while testing patch 1, tested on BCM57504 - boot; set rx-copybreak
to 1024 with the link down (CONFIG_DMA_API_DEBUG); ifup.

Fixes: 152f4da05aee ("bnxt_en: add support for rx-copybreak ethtool command")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 622e89587e5d..ef4bea4a0f8c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -4601,6 +4601,7 @@ static int bnxt_set_tunable(struct net_device *dev,
 			if (netif_running(dev))
 				return -EBUSY;
 			bp->rx_copybreak = rx_copybreak;
+			bnxt_set_ring_params(bp);
 		}
 		return 0;
 	case ETHTOOL_PFC_PREVENTION_TOUT:
-- 
2.56.0

[PATCH net 3/5] eth: bnxt: don't leak buffers when XDP agg buffer allocation fails

[Jakub Kicinski]

If a replacement agg buffer can't be allocated for a packet received
with XDP attached, bnxt_rx_pkt() drops the packet without returning
its head buffer to the ring. The head buffer leaks, and since rx_prod
still advances, the device gets a BD with a stale DMA address which
points at a buffer the driver no longer owns. When that BD completes
the driver finds no buffer in the slot.

The agg buffers consumed before the failing one have already been
replaced on the agg ring, they are referenced only by the frags of
the xdp_buff, which nobody frees. bnxt_xdp_buff_frags_free() would
skip them since the frags flag is set only on success, so set it
upfront.

Pointed out in LLM review of the first patch of the series.
Tested on a BCM57504 at MTU 9000 with a frags-capable XDP_PASS
program, receiving 8000B ICMPv6 echo requests (a head and two agg
buffers each) while page_pool_alloc_netmems() called from
bnxt_rx_agg_netmems_xdp() failed half of the time (bpftrace
override()). 44.7k packets were dropped, the page pools' inflight
count stayed flat and there were no KASAN or DEBUG_PAGEALLOC splats.

Fixes: 4c6c123c9af9 ("bnxt: adding bnxt_rx_agg_pages_xdp for aggregated xdp")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 2e7276801fdd..4607c31eedf8 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -1427,16 +1427,19 @@ static u32 bnxt_rx_agg_netmems_xdp(struct bnxt *bp,
 	struct skb_shared_info *shinfo = xdp_get_shared_info_from_buff(xdp);
 	u32 total_frag_len = 0;
 
-	if (!xdp_buff_has_frags(xdp))
+	if (!xdp_buff_has_frags(xdp)) {
+		xdp_buff_set_frags_flag(xdp);
 		shinfo->nr_frags = 0;
+	}
 
 	total_frag_len = __bnxt_rx_agg_netmems(bp, cpr, idx, agg_bufs, tpa,
 					       NULL, xdp);
-	if (total_frag_len) {
-		xdp_buff_set_frags_flag(xdp);
-		shinfo->nr_frags = agg_bufs;
-		shinfo->xdp_frags_size = total_frag_len;
+	if (!total_frag_len) {
+		bnxt_xdp_buff_frags_free(cpr->bnapi->rx_ring, xdp);
+		return 0;
 	}
+	shinfo->nr_frags = agg_bufs;
+	shinfo->xdp_frags_size = total_frag_len;
 	return total_frag_len;
 }
 
@@ -2293,8 +2296,10 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 							       cp_cons,
 							       agg_bufs,
 							       false);
-			if (!frag_len)
+			if (!frag_len) {
+				bnxt_reuse_rx_data(rxr, cons, data);
 				goto oom_next_rx;
+			}
 
 		}
 		xdp_active = true;
-- 
2.56.0

[PATCH net 4/5] eth: bnxt: free XDP frags when xdp_do_redirect() fails

[Jakub Kicinski]

When xdp_do_redirect() fails bnxt_rx_xdp() recycles the head page but
not the frags of a multi-buffer frame. Their agg ring slots have
already been refilled, so the pages leak, and the page pool can't be
destroyed until they are returned, which will never happen. Redirect
to an AF_XDP socket in copy mode fails this way with -ENOMEM whenever
the socket's fill ring runs out of buffers.
A socket bound without XDP_USE_SG also fails with -ENOSPC for every
frame that doesn't fit in a single chunk.

All the other drop paths in this function free the frags.

Pointed out in LLM review of the first patch of the series.
Tested on a BCM57504 at MTU 9000 by redirecting 60k 8000B ICMPv6
echo requests (a head and two agg buffers each) to lo, which can't
transmit XDP frames. The page pools' inflight count stayed flat
rather than growing by two pages per frame.

Fixes: a7559bc8c17c ("bnxt: support transmit and free of aggregation buffers")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
index 96845c8646e0..161e0dd62be6 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
@@ -307,6 +307,7 @@ bool bnxt_rx_xdp(struct bnxt *bp, struct bnxt_rx_ring_info *rxr, u16 cons,
 
 		if (xdp_do_redirect(bp->dev, xdp, xdp_prog)) {
 			trace_xdp_exception(bp->dev, xdp_prog, act);
+			bnxt_xdp_buff_frags_free(rxr, xdp);
 			page_pool_recycle_direct(rxr->page_pool, page);
 			return true;
 		}
-- 
2.56.0

[PATCH net 5/5] eth: bnxt: hand XDP frags over to the copybreak skb

[Jakub Kicinski]

With a multi-buffer XDP program attached, a packet only has agg
buffers if it filled its head buffer, which in page mode holds most
of a page. The program can still move the start of the packet forward
with bpf_xdp_adjust_head() and leave at most rx_copybreak bytes in the
head. bnxt_rx_pkt() then copies the head into a new skb, puts the head
page back on the ring, and calls bnxt_xdp_build_skb() on the copy.
bnxt_xdp_build_skb() only sets nr_frags and the skb's length
accounting, because it expects an skb built around the head page,
sharing its shinfo with the xdp_buff. The copy ends up with nr_frags
set but no frag descriptors, and the stack later reads and releases
whatever its frags array happens to contain. The real frags leak.

Copy the frag descriptors to the new skb in bnxt_copy_xdp(), so that
bnxt_xdp_build_skb() finds the frags where it expects them. The frags
now belong to the skb, and the head page goes back on the ring as
before.

Pointed out in LLM review of the first patch of the series.
Confirmed by reading the code, the unfixed kernel was not run with
the reproducer below.
Tested on a BCM57504 at MTU 9000 with rx-copybreak raised to 1024
and a frags-capable XDP program moving the start of multi-buffer
frames 3000B forward with bpf_xdp_adjust_head(), receiving 8000B
ICMPv6 echo requests (a head and two agg buffers each). All 60k
frames took the copybreak path with frags attached, the sampled skbs
reached GRO with len=4998 data_len=4492 nr_frags=2, and were freed by
the stack with the page pools' inflight count staying flat, no KASAN
or DEBUG_PAGEALLOC splats.

Fixes: 1dc4c557bfed ("bnxt: adding bnxt_xdp_build_skb to build skb from multibuffer xdp_buff")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 4607c31eedf8..757c7266ce15 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -1511,6 +1511,14 @@ static struct sk_buff *bnxt_copy_xdp(struct bnxt_napi *bnapi,
 		__skb_pull(skb, metasize);
 	}
 
+	if (unlikely(xdp_buff_has_frags(xdp))) {
+		struct skb_shared_info *sinfo;
+
+		sinfo = xdp_get_shared_info_from_buff(xdp);
+		memcpy(skb_shinfo(skb)->frags, sinfo->frags,
+		       sinfo->nr_frags * sizeof(skb_frag_t));
+	}
+
 	return skb;
 }
 
-- 
2.56.0

Re: [PATCH net 1/5] eth: bnxt: recycle agg buffers when Rx head buffer allocation fails

[Michael Chan]

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

On Tue, Oct 27, 2026 at 4:49 PM Jakub Kicinski <kuba@kernel.org> wrote:

> Note the odd split of ownership here: rx_skb_func() only deals with
> the head buffer, the agg buffers stay with the caller in whatever
> state they are in, still on the ring without XDP, or frags of the
> xdp_buff with XDP. This complicates rx_skb_func() cleaning up all
> the buffers (head and frags), which would be more intuitive.

Yeah, rx_skb_func() only deals with the head buffer and it needs 7
function parameters already.  Letting it handle agg bufs would make it
more complicated.

I also double checked the TPA code path (copybreak and non-copybreak)
and it doesn't have this issue of leaking agg buffers during OOM.

Thanks for the patches.  I will review the whole set more thoroughly tomorrow.

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

Re: [PATCH net 1/5] eth: bnxt: recycle agg buffers when Rx head buffer allocation fails

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] bnxt_rx_multi_page_skb() now re-posts the head page to the device
  without dma_sync_single_for_device() when napi_build_skb() fails.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch adds
  a new consumer of it.
- [Medium] At this commit, the XDP multi-buffer path in bnxt_rx_pkt()
  still leaks buffers when bnxt_rx_agg_netmems_xdp() fails partway.

--

eth: bnxt: recycle agg buffers when Rx head buffer allocation fails

This commit adds an oom_next_rx_agg label to bnxt_rx_pkt(). When the
rx_skb_func() head buffer allocation fails, the label returns the
aggregation buffers to the agg ring, or frees the XDP frags, the same way
the copybreak failure path already does. In bnxt_rx_multi_page_skb(), a
napi_build_skb() failure now releases the newly allocated replacement
buffer and puts the original head page back on the ring, so the XDP frags
in its shinfo are still there for the caller's cleanup.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 0682060b048ce..2e7276801fddc 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -1215,7 +1215,15 @@ static struct sk_buff *bnxt_rx_multi_page_skb(struct bnxt *bp,
>  				bp->rx_dir);
>  	skb = napi_build_skb(frag_start, rxr->rx_page_size);
>  	if (!skb) {
> -		page_pool_recycle_direct(rxr->page_pool, page);
> +		struct page *new_page;
> +
> +		new_page = rxr->rx_buf_ring[RING_RX(bp, prod)].data;
> +		/* The caller may still need the XDP frags from shinfo at the
> +		 * end of the head page. Release the new buffer instead and
> +		 * put the head back on the ring.
> +		 */

[Severity: High]
This is a pre-existing issue, not one this patch introduced. It goes back
to 4c6c123c9af9, but this comment relies on it: can the frags in shinfo
still be trusted at this point when DMA goes through swiotlb, or on
non-coherent systems?

In XDP multi-buffer mode, bnxt_xdp_buff_init() syncs only the packet
payload for the CPU:

bnxt_xdp_buff_init()
    dma_sync_single_for_cpu(&pdev->dev, mapping + offset, len, bp->rx_dir);

bnxt_rx_agg_netmems_xdp()->__bnxt_rx_agg_netmems() then writes
shinfo->frags[] and nr_frags at the end of the head page, while that
region is still owned by the device.

Afterwards, bnxt_rx_multi_page_skb() syncs the whole buffer, including
the shinfo:

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

rx_dir is DMA_BIDIRECTIONAL here, so __swiotlb_sync_single_for_cpu()
copies the bounce buffer back over the original page. In page mode the
device never writes the shinfo area of the bounce buffer. The frags the
CPU just wrote could therefore be overwritten with stale frags from an
earlier packet. On non-coherent systems, a cache invalidate could drop
them in the same way.

With this patch, the napi_build_skb() failure path continues to
oom_next_rx_agg->bnxt_xdp_buff_frags_free(), which calls
page_pool_recycle_direct() on every page listed in that shinfo. Could
that recycle pages still owned by earlier skbs, while the frags that were
actually received leak?

The success path reads the same data through
bnxt_xdp_build_skb()->xdp_update_skb_frags_info().

Would it work to sync only the range the device wrote, or to do the
full-page sync before the frags are written into shinfo?

> +		page_pool_recycle_direct(rxr->page_pool, new_page);
> +		bnxt_reuse_rx_data(rxr, cons, data);

[Severity: Low]
Does the head page need a dma_sync_single_for_device() before it goes
back on the ring?

The whole buffer was synced for the CPU just above. The old
page_pool_recycle_direct() call synced the page for the device, because
the pool is created with PP_FLAG_DMA_SYNC_DEV. bnxt_reuse_rx_data() only
copies the old DMA address into the producer BD, so that sync no longer
happens.

The CPU also keeps writing to the head page after it has been re-posted.
On the oom_next_rx_agg path, bnxt_xdp_buff_frags_free() sets
shinfo->nr_frags = 0.

For comparison, bnxt_copy_data() pairs both syncs before bnxt_rx_pkt()
reuses the head:

    dma_sync_single_for_cpu(&pdev->dev, mapping, bp->rx_copybreak,
                            bp->rx_dir);

    memcpy(skb->data - NET_IP_ALIGN, data - NET_IP_ALIGN,
           len + NET_IP_ALIGN);

    dma_sync_single_for_device(&pdev->dev, mapping, bp->rx_copybreak,
                               bp->rx_dir);

The XDP_DROP and XDP_TX ring-full branches in bnxt_rx_xdp() already reuse
the head without a sync for the device, so the new code matches them. It
does drop a sync that the old path did, though.

>  		return NULL;
>  	}
>  	skb_mark_for_recycle(skb);

[ ... ]

> @@ -2424,6 +2423,14 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
>  
>  	return rc;
>  
> +oom_next_rx_agg:
> +	if (agg_bufs) {
> +		if (!xdp_active)
> +			bnxt_reuse_rx_agg_bufs(cpr, cp_cons, 0, agg_bufs,
> +					       false);
> +		else
> +			bnxt_xdp_buff_frags_free(rxr, &bnxt_xdp.xdp);
> +	}

[Severity: Medium]
This isn't a bug introduced by this patch, but the new label doesn't
cover a partial failure inside bnxt_rx_agg_netmems_xdp(). Are frags
0..i-1 leaked in that case?

If bnxt_alloc_rx_netmem() fails at index i > 0, __bnxt_rx_agg_netmems()
has already put frags 0..i-1 into the head page's shinfo and refilled
their agg ring slots. It then reposts only entries i..agg_bufs-1 and
returns 0:

    --shinfo->nr_frags;
    cons_rx_buf->netmem = netmem;
    ...
    rxr->rx_agg_prod = prod;
    bnxt_reuse_rx_agg_bufs(cpr, idx, i, agg_bufs - i, tpa);
    return 0;

bnxt_rx_agg_netmems_xdp() sets the frags flag only when total_frag_len
is non-zero:

    if (total_frag_len) {
        xdp_buff_set_frags_flag(xdp);
        shinfo->nr_frags = agg_bufs;
        shinfo->xdp_frags_size = total_frag_len;
    }

So bnxt_xdp_buff_frags_free() would skip these frags even if it were
called. At this revision, bnxt_rx_pkt() also doesn't put the head buffer
back on the ring:

    if (!frag_len)
        goto oom_next_rx;

A later commit in this series, "eth: bnxt: don't leak buffers when XDP
agg buffer allocation fails", appears to fix this. It sets the frags
flag before the loop, frees the frags when total_frag_len is 0, and adds
bnxt_reuse_rx_data() before goto oom_next_rx. With that commit applied,
the end state of the series no longer has this problem.

>  oom_next_rx:
>  	cpr->sw_stats->rx.rx_oom_discards += 1;
>  	rc = -ENOMEM;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027234940.1267225-1-kuba%40kernel.org

Re: [PATCH net 2/5] eth: bnxt: resize Rx head buffers when rx-copybreak changes

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  bnxt_copy_data() (drivers/net/ethernet/broadcom/bnxt/bnxt.c) syncs…

--

eth: bnxt: resize Rx head buffers when rx-copybreak changes

This patch makes bnxt_set_tunable() call bnxt_set_ring_params() after it
changes bp->rx_copybreak, so the Rx head buffers are resized before the
next open. The goal is to stop bnxt_copy_data() from syncing rx_copybreak
bytes past the end of head buffers that are too small.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index 622e89587e5db..ef4bea4a0f8cc 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -4601,6 +4601,7 @@ static int bnxt_set_tunable(struct net_device *dev,
>  			if (netif_running(dev))
>  				return -EBUSY;
>  			bp->rx_copybreak = rx_copybreak;
> +			bnxt_set_ring_params(bp);

[Severity: High]

Does this cover the case where no aggregation ring is in use?

The commit message says bnxt_set_ring_params() sizes the head buffers to
fit at least rx_copybreak bytes. The rx_copybreak floor only exists inside
the agg_factor branch, though:

drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_set_ring_params() {
    ...
	if (agg_factor) {
		...
		if (BNXT_RX_PAGE_MODE(bp)) {
			...
		} else {
			rx_size = max3(BNXT_MIN_RX_HDR_BUF,
				       bp->rx_copybreak,
				       bp->dev->cfg_pending->hds_thresh);
			...
		}
	}

	bp->rx_buf_use_size = rx_size;
	bp->rx_buf_size = rx_space;
    ...
}

agg_factor is 0 when TPA is off, HDS is off and rx_space <= PAGE_SIZE.
That happens after ethtool -K gro-hw/lro off, in a kdump kernel, or with
BNXT_FLAG_NO_AGG_RINGS. In that case the sizes come only from the MTU:

	rx_size = SKB_DATA_ALIGN(bp->dev->mtu + ETH_HLEN + NET_IP_ALIGN + 8);

	rx_space = rx_size + ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) +
		SKB_DATA_ALIGN(sizeof(struct skb_shared_info));

bnxt_set_tunable() still accepts any rx_copybreak up to
BNXT_MAX_RX_HDR_BUF. bnxt_copy_data() still syncs a fixed
bp->rx_copybreak bytes:

	dma_sync_single_for_cpu(&pdev->dev, mapping, bp->rx_copybreak,
				bp->rx_dir);
	...
	dma_sync_single_for_device(&pdev->dev, mapping, bp->rx_copybreak,
				   bp->rx_dir);

__bnxt_alloc_rx_frag() packs rx_buf_size fragments into one head_pool
page, and each mapping starts NET_SKB_PAD bytes into its fragment. Two
examples on x86-64 with rx-copybreak set to 1024:

- MTU 60: rx_buf_size is 704, so each sync runs 384 bytes into the next
  fragment.
- MTU 400: rx_buf_size is 1024, with four fragments per page. The sync
  for the last fragment ends 64 bytes past the page and outside its DMA
  mapping.

The BD length here is rx_buf_use_size, which is <= rx_copybreak, so every
received packet goes through bnxt_rx_pkt()->bnxt_copy_skb()->
bnxt_copy_data().

Isn't the oversized sync from the commit message still reachable on this
path? With swiotlb or non-coherent DMA it could still corrupt neighbouring
buffers. The later patches in the series don't seem to touch
bnxt_set_ring_params() or the sync length in bnxt_copy_data().

Would any of these be a more complete fix?

- bound the sync to len + NET_IP_ALIGN
- apply the rx_copybreak floor on the non-agg path as well
- reject rx_copybreak values larger than the current head buffer

[Severity: Medium]

This is a pre-existing issue, not introduced by this patch. In
bnxt_copy_data() the CPU sync looks shorter than the memcpy that follows
it:

	dma_sync_single_for_cpu(&pdev->dev, mapping, bp->rx_copybreak,
				bp->rx_dir);

	memcpy(skb->data - NET_IP_ALIGN, data - NET_IP_ALIGN,
	       len + NET_IP_ALIGN);

bp->rx_offset - bp->rx_dma_offset == NET_IP_ALIGN, so data - NET_IP_ALIGN
is rx_buf->mapping. When NET_IP_ALIGN == 2, bnxt_init_one_rx_ring_rxbd()
sets RX_BD_FLAGS_SOP, and the hardware places the packet at mapping + 2.

Take a copybreak packet whose len is rx_copybreak or rx_copybreak - 1. The
memcpy reads up to rx_copybreak + 2 bytes from the mapping, so the last 1-2
bytes of the packet were never synced.

Could those bytes come from the stale original buffer with swiotlb, or from
a stale cache line with non-coherent DMA? bnxt reports
CHECKSUM_UNNECESSARY for these packets, so the stack would not catch the
corrupted tail.

This only affects architectures that keep NET_IP_ALIGN at 2. x86, arm64 and
powerpc define it as 0.

A sync length of len + NET_IP_ALIGN would fix this and would also bound the
overrun described above.

>  		}
>  		return 0;
>  	case ETHTOOL_PFC_PREVENTION_TOUT:

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027234940.1267225-1-kuba%40kernel.org

Re: [PATCH net 1/5] eth: bnxt: recycle agg buffers when Rx head buffer allocation fails

[Jakub Kicinski]

On Wed, 28 Oct 2026 23:52:43 +0000 netdev-bot+sashiko@kernel.org wrote:
> Low priority / nit picks for your consideration:
> 
> - [Low] bnxt_rx_multi_page_skb() now re-posts the head page to the device
>   without dma_sync_single_for_device() when napi_build_skb() fails.

This is what I flagged in the cover letter as outstanding bugs in DMA
sync which I have separate series for.

> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the patch adds
>   a new consumer of it.

Same.

> - [Medium] At this commit, the XDP multi-buffer path in bnxt_rx_pkt()
>   still leaks buffers when bnxt_rx_agg_netmems_xdp() fails partway.

As bot realizes this is fixed later in the series.

Re: [PATCH net 2/5] eth: bnxt: resize Rx head buffers when rx-copybreak changes

[Jakub Kicinski]

On Wed, 28 Oct 2026 23:52:45 +0000 netdev-bot+sashiko@kernel.org wrote:
> - [High] The fix is incomplete.

:S Maaayybeee?  I think the bot is over-focusing on the DMA warning hit
in the commit msg. It's true that we may not cover all the corner cases
for that DMA warning. But I deferred the DMA syncing fixes as mentioned
in the cover letter.

Here I was purely trying to fix the AFAICT obviously missing
bnxt_set_ring_params() call after changing rx_copybreak.

But if anyone disagrees we can just skip this patch when applying, and
put it to the DMA sync series. I included it here because without it
I couldn't test patch 5 without triggering the splat first.

Re: [PATCH net 2/5] eth: bnxt: resize Rx head buffers when rx-copybreak changes

[Michael Chan]

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

On Wed, Oct 28, 2026 at 5:58 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 28 Oct 2026 23:52:45 +0000 netdev-bot+sashiko@kernel.org wrote:
> > - [High] The fix is incomplete.
>
> :S Maaayybeee?  I think the bot is over-focusing on the DMA warning hit
> in the commit msg. It's true that we may not cover all the corner cases
> for that DMA warning. But I deferred the DMA syncing fixes as mentioned
> in the cover letter.
>
> Here I was purely trying to fix the AFAICT obviously missing
> bnxt_set_ring_params() call after changing rx_copybreak.
>
> But if anyone disagrees we can just skip this patch when applying, and
> put it to the DMA sync series. I included it here because without it
> I couldn't test patch 5 without triggering the splat first.

No objection from me.  The head buffer sizing fix is more important
than the remaining DMA sync warning.

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

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

Re: [PATCH net 4/5] eth: bnxt: free XDP frags when xdp_do_redirect() fails

[Michael Chan]

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

On Tue, Oct 27, 2026 at 4:49 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> When xdp_do_redirect() fails bnxt_rx_xdp() recycles the head page but
> not the frags of a multi-buffer frame. Their agg ring slots have
> already been refilled, so the pages leak, and the page pool can't be
> destroyed until they are returned, which will never happen. Redirect
> to an AF_XDP socket in copy mode fails this way with -ENOMEM whenever
> the socket's fill ring runs out of buffers.
> A socket bound without XDP_USE_SG also fails with -ENOSPC for every
> frame that doesn't fit in a single chunk.
>
Note that in the opposite direction of XDP_REDIRECT to bnxt, it will
not transmit multi-buffer packets.  Only the head buffer will be
transmitted but the whole buffer should be freed correctly.  Andy, can
you confirm and maybe provide some history on this?

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

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

Re: [PATCH net 0/5] eth: bnxt: fix Rx buffer leaks under OOM

[Michael Chan]

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

On Tue, Oct 27, 2026 at 4:49 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Commit bd3beb5c8691 ("eth: bnxt: disable rx-copybreak by default")
> changed the defaults putting users on buggy(ier) driver paths.
>
> Fix various ways bnxt may leak buffers under OOM. The first patch
> fixes the real bug that was hit in production. Second patch fixes
> an issue I hit while testing the first one. The next 3 patches
> fix other Rx buffer leaks pointed out by an LLM during review.

For the whole series:
Reviewed-by: Michael Chan <michael.chan@broadcom.com>

I've also asked Andy to review these patches since a number of them
are related to XDP multi-buffer and XDP_REDIRECT.  Thanks.

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