[PATCH net] igc: track rx frame boundaries independent of skbs

[PATCH net] igc: track rx frame boundaries independent of skbs

[Paul Moses]

TSIP remains set on continuation descriptors, but the inline timestamp
header exists only in the first buffer. Stripping it from every
descriptor truncates split frames and can underflow short buffers.

skb ownership cannot identify frame boundaries when XDP consumes the
first buffer or AF_XDP uses zero-copy.

Track rx_in_frame per ring and update it as descriptors are consumed.
Strip timestamps only from first buffers, reject unsupported split
frames before XDP and drain continuations. Account discarded
descriptors against NAPI work without counting successful RX.

Fixes: e1ed4f92a625 ("igc: Refactor Rx timestamp handling")
Fixes: 26575105d6ed ("igc: Add initial XDP support")
Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy")
Cc: stable@vger.kernel.org
Signed-off-by: Paul Moses <p@1g4.org>
---
- This supersedes "igc: only strip RX timestamp header from first buffer"
  following feedback from Simon Horman, Maciej Fijalkowski and Sashiko.

Changes:
- track frame boundaries independently of skb ownership
- guard timestamp stripping and drain unsupported split frames
- cover the AF_XDP zero-copy RX path
---
 drivers/net/ethernet/intel/igc/igc.h      |  1 +
 drivers/net/ethernet/intel/igc/igc_main.c | 87 ++++++++++++++++-------
 2 files changed, 64 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 17f213cc93e4..635d49701e62 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -198,6 +198,7 @@ struct igc_ring {
 			struct igc_rx_packet_stats pkt_stats;
 			struct u64_stats_sync rx_syncp;
 			struct sk_buff *skb;
+			bool rx_in_frame;
 		};
 	};
 
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 1fb5f3cbe93c..9ca5558c762c 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -474,6 +474,7 @@ static void igc_clean_rx_ring(struct igc_ring *ring)
 		igc_clean_rx_ring_page_shared(ring);
 
 	clear_ring_uses_large_buffer(ring);
+	ring->rx_in_frame = false;
 
 	ring->next_to_alloc = 0;
 	ring->next_to_clean = 0;
@@ -673,6 +674,7 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
 	/* reset next-to- use/clean to place SW in sync with hardware */
 	ring->next_to_clean = 0;
 	ring->next_to_use = 0;
+	ring->rx_in_frame = false;
 
 	if (ring->xsk_pool)
 		buf_size = xsk_pool_get_rx_frame_size(ring->xsk_pool);
@@ -2111,10 +2113,8 @@ static bool igc_can_reuse_rx_page(struct igc_rx_buffer *rx_buffer,
  * @rx_ring: Rx ring being processed
  * @rx_desc: Rx descriptor for current buffer
  *
- * This function updates next to clean.  If the buffer is an EOP buffer
- * this function exits returning false, otherwise it will place the
- * sk_buff in the next buffer to be chained and return true indicating
- * that this is in fact a non-EOP buffer.
+ * Advance next to clean and retain frame position for the next descriptor.
+ * Return true if the consumed descriptor does not end the frame.
  */
 static bool igc_is_non_eop(struct igc_ring *rx_ring,
 			   union igc_adv_rx_desc *rx_desc)
@@ -2127,10 +2127,8 @@ static bool igc_is_non_eop(struct igc_ring *rx_ring,
 
 	prefetch(IGC_RX_DESC(rx_ring, ntc));
 
-	if (likely(igc_test_staterr(rx_desc, IGC_RXD_STAT_EOP)))
-		return false;
-
-	return true;
+	rx_ring->rx_in_frame = !igc_test_staterr(rx_desc, IGC_RXD_STAT_EOP);
+	return unlikely(rx_ring->rx_in_frame);
 }
 
 /**
@@ -2601,7 +2599,7 @@ static void igc_update_rx_stats(struct igc_q_vector *q_vector,
 
 static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 {
-	unsigned int total_bytes = 0, total_packets = 0;
+	unsigned int total_bytes = 0, total_packets = 0, work_done = 0;
 	struct igc_adapter *adapter = q_vector->adapter;
 	struct igc_ring *rx_ring = q_vector->rx.ring;
 	struct sk_buff *skb = rx_ring->skb;
@@ -2609,13 +2607,15 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 	int xdp_status = 0, rx_buffer_pgcnt;
 	int xdp_res = 0;
 
-	while (likely(total_packets < budget)) {
+	while (likely(work_done < budget)) {
 		struct igc_xdp_buff ctx = { .rx_ts = NULL };
+		bool first = !rx_ring->rx_in_frame;
 		struct igc_rx_buffer *rx_buffer;
 		union igc_adv_rx_desc *rx_desc;
 		unsigned int size, truesize;
 		int pkt_offset = 0;
 		void *pktbuf;
+		bool tsip;
 
 		/* return some buffers to hardware, one at a time is too slow */
 		if (cleaned_count >= IGC_RX_BUFFER_WRITE) {
@@ -2635,21 +2635,34 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 		dma_rmb();
 
 		rx_buffer = igc_get_rx_buffer(rx_ring, size, &rx_buffer_pgcnt);
+		tsip = first && igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP);
+
+		if ((!first && !skb) ||
+		    (first && igc_xdp_is_enabled(adapter) &&
+		     !igc_test_staterr(rx_desc, IGC_RXD_STAT_EOP)) ||
+		    unlikely(tsip && size < IGC_TS_HDR_LEN)) {
+			if (first)
+				dev_core_stats_rx_dropped_inc(rx_ring->netdev);
+			xdp_res = IGC_XDP_CONSUMED;
+			goto xdp_done;
+		}
+
 		truesize = igc_get_rx_frame_truesize(rx_ring, size);
 
 		pktbuf = page_address(rx_buffer->page) + rx_buffer->page_offset;
 
-		if (igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)) {
+		if (tsip) {
 			ctx.rx_ts = pktbuf;
 			pkt_offset = IGC_TS_HDR_LEN;
 			size -= IGC_TS_HDR_LEN;
 		}
 
-		if (igc_fpe_is_pmac_enabled(adapter) &&
+		if (first && igc_fpe_is_pmac_enabled(adapter) &&
 		    igc_fpe_handle_mpacket(adapter, rx_desc, size, pktbuf + pkt_offset)) {
 			/* Advance the ring next-to-clean */
 			igc_is_non_eop(rx_ring, rx_desc);
 			cleaned_count++;
+			work_done++;
 			continue;
 		}
 
@@ -2664,6 +2677,12 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 			xdp_res = igc_xdp_run_prog(adapter, &ctx.xdp);
 		}
 
+		if (xdp_res) {
+			total_bytes += size;
+			total_packets++;
+		}
+
+xdp_done:
 		if (xdp_res) {
 			switch (xdp_res) {
 			case IGC_XDP_CONSUMED:
@@ -2676,8 +2695,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 				break;
 			}
 
-			total_packets++;
-			total_bytes += size;
+			work_done++;
 		} else if (skb)
 			igc_add_rx_frag(rx_ring, rx_buffer, skb, size);
 		else if (ring_uses_build_skb(rx_ring))
@@ -2700,8 +2718,15 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 		if (igc_is_non_eop(rx_ring, rx_desc))
 			continue;
 
+		if (xdp_res) {
+			skb = NULL;
+			continue;
+		}
+
+		work_done++;
+
 		/* verify the packet layout is correct */
-		if (xdp_res || igc_cleanup_headers(rx_ring, rx_desc, skb)) {
+		if (igc_cleanup_headers(rx_ring, rx_desc, skb)) {
 			skb = NULL;
 			continue;
 		}
@@ -2717,7 +2742,6 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 		/* reset skb pointer */
 		skb = NULL;
 
-		/* update budget accounting */
 		total_packets++;
 	}
 
@@ -2732,7 +2756,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 	if (cleaned_count)
 		igc_alloc_rx_buffers(rx_ring, cleaned_count);
 
-	return total_packets;
+	return work_done;
 }
 
 static struct sk_buff *igc_construct_skb_zc(struct igc_ring *ring,
@@ -2797,10 +2821,10 @@ static struct igc_xdp_buff *xsk_buff_to_igc_ctx(struct xdp_buff *xdp)
 
 static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
 {
+	int total_bytes = 0, total_packets = 0, work_done = 0;
 	struct igc_adapter *adapter = q_vector->adapter;
 	struct igc_ring *ring = q_vector->rx.ring;
 	u16 cleaned_count = igc_desc_unused(ring);
-	int total_bytes = 0, total_packets = 0;
 	u16 ntc = ring->next_to_clean;
 	struct bpf_prog *prog;
 	bool failure = false;
@@ -2810,11 +2834,13 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
 
 	prog = READ_ONCE(adapter->xdp_prog);
 
-	while (likely(total_packets < budget)) {
+	while (likely(work_done < budget)) {
+		bool first = !ring->rx_in_frame;
 		union igc_adv_rx_desc *desc;
 		struct igc_rx_buffer *bi;
 		struct igc_xdp_buff *ctx;
 		unsigned int size;
+		bool eop, tsip;
 		int res;
 
 		desc = IGC_RX_DESC(ring, ntc);
@@ -2829,11 +2855,20 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
 		dma_rmb();
 
 		bi = &ring->rx_buffer_info[ntc];
+		tsip = first && igc_test_staterr(desc, IGC_RXDADV_STAT_TSIP);
+		eop = igc_test_staterr(desc, IGC_RXD_STAT_EOP);
+
+		if (!first || !eop || unlikely(tsip && size < IGC_TS_HDR_LEN)) {
+			if (first)
+				dev_core_stats_rx_dropped_inc(ring->netdev);
+			res = IGC_XDP_CONSUMED;
+			goto xdp_done;
+		}
 
 		ctx = xsk_buff_to_igc_ctx(bi->xdp);
 		ctx->rx_desc = desc;
 
-		if (igc_test_staterr(desc, IGC_RXDADV_STAT_TSIP)) {
+		if (tsip) {
 			ctx->rx_ts = bi->xdp->data;
 
 			bi->xdp->data += IGC_TS_HDR_LEN;
@@ -2851,6 +2886,10 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
 		xsk_buff_dma_sync_for_cpu(bi->xdp);
 
 		res = __igc_xdp_run_prog(adapter, prog, bi->xdp);
+		total_bytes += size;
+		total_packets++;
+
+xdp_done:
 		switch (res) {
 		case IGC_XDP_PASS:
 			igc_dispatch_skb_zc(q_vector, desc, ctx);
@@ -2865,8 +2904,8 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
 		}
 
 		bi->xdp = NULL;
-		total_bytes += size;
-		total_packets++;
+		ring->rx_in_frame = !eop;
+		work_done++;
 		cleaned_count++;
 		ntc++;
 		if (ntc == ring->count)
@@ -2889,10 +2928,10 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
 			xsk_set_rx_need_wakeup(ring->xsk_pool);
 		else
 			xsk_clear_rx_need_wakeup(ring->xsk_pool);
-		return total_packets;
+		return work_done;
 	}
 
-	return failure ? budget : total_packets;
+	return failure ? budget : work_done;
 }
 
 static void igc_update_tx_stats(struct igc_q_vector *q_vector,

Re: [PATCH net] igc: track rx frame boundaries independent of skbs

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] igc: track rx frame boundaries independent of skbs

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (2) · Medium: 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).

- [Medium] The new split-frame discard paths leave frames out of
  rx_packets and rx_bytes, which contradicts the rtnl_link_stats64…

Pre-existing issues:
- [High] In igc_clean_rx_irq(), the FPE mPacket early exit leaks the RX
  page and its DMA mapping.
- [High] igc_xdp_set_prog(), igc_xdp_enable_pool() and
  igc_xdp_disable_pool() tear down and rebuild the RX ring while NAPI…

--

igc: track rx frame boundaries independent of skbs

The patch adds a per-ring rx_in_frame flag so the inline TSIP timestamp
header is stripped only from the first buffer of a frame. Split frames
that are not supported (XDP enabled, AF_XDP zero-copy) are rejected
before XDP runs, and their continuation descriptors are drained. The RX
poll functions now return a work_done count that includes discarded
descriptors, instead of total_packets.

> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 1fb5f3cbe93c5..9ca5558c762c9 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -474,6 +474,7 @@ static void igc_clean_rx_ring(struct igc_ring *ring)
>  		igc_clean_rx_ring_page_shared(ring);
>  
>  	clear_ring_uses_large_buffer(ring);
> +	ring->rx_in_frame = false;
>  
>  	ring->next_to_alloc = 0;
>  	ring->next_to_clean = 0;
> @@ -673,6 +674,7 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
>  	/* reset next-to- use/clean to place SW in sync with hardware */
>  	ring->next_to_clean = 0;
>  	ring->next_to_use = 0;
> +	ring->rx_in_frame = false;
>  
>  	if (ring->xsk_pool)
>  		buf_size = xsk_pool_get_rx_frame_size(ring->xsk_pool);

[Severity: High]
This isn't a bug introduced by this patch, but these new resets assume
NAPI is not polling the ring. Do the XDP reconfiguration paths actually
guarantee that?

igc_xdp_set_prog(), igc_xdp_enable_pool() and igc_xdp_disable_pool() all
clean the ring before they disable NAPI:

drivers/net/ethernet/intel/igc/igc_xdp.c:igc_xdp_set_prog() {
    ...
		for (i = 0; i < adapter->num_rx_queues; i++) {
			igc_disable_rx_ring(adapter->rx_ring[i]);
			igc_disable_tx_ring(adapter->tx_ring[i]);
			napi_disable(&adapter->rx_ring[i]->q_vector->napi);
		}
    ...
			napi_enable(&adapter->rx_ring[i]->q_vector->napi);
			igc_enable_tx_ring(adapter->tx_ring[i]);
			igc_enable_rx_ring(adapter->rx_ring[i]);
    ...
}

igc_disable_rx_ring() does only an RXDCTL write before it calls
igc_clean_rx_ring(). Also, napi_enable() runs before
igc_enable_rx_ring()->igc_configure_rx_ring().

Suppose igc_clean_rx_irq() is running on another CPU while
igc_clean_rx_ring_page_shared() calls dev_kfree_skb(rx_ring->skb) and
drains the pages. Could the poll then call igc_add_rx_frag() on the freed
skb, or drop page references a second time through igc_put_rx_buffer()?

When the poll exits, it also writes rx_ring->skb, next_to_clean and
rx_in_frame back, after the reset. igc_configure_rx_ring() resets
rx_in_frame but not rx_ring->skb. A later poll could then see
first == true with a stale non-NULL skb, and the new (!first && !skb)
check would not catch that.

Triggering this needs a CAP_NET_ADMIN user to attach or detach an XDP
program, or to bind or unbind an AF_XDP pool while XDP is loaded, while
RX traffic keeps a poll running on another CPU.

[ ... ]

> @@ -2635,21 +2635,34 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>  		dma_rmb();
>  
>  		rx_buffer = igc_get_rx_buffer(rx_ring, size, &rx_buffer_pgcnt);
> +		tsip = first && igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP);
> +
> +		if ((!first && !skb) ||
> +		    (first && igc_xdp_is_enabled(adapter) &&
> +		     !igc_test_staterr(rx_desc, IGC_RXD_STAT_EOP)) ||
> +		    unlikely(tsip && size < IGC_TS_HDR_LEN)) {
> +			if (first)
> +				dev_core_stats_rx_dropped_inc(rx_ring->netdev);
> +			xdp_res = IGC_XDP_CONSUMED;
> +			goto xdp_done;
> +		}
> +

[Severity: Medium]
Should the frames dropped here still count toward rx_packets and
rx_bytes?

include/uapi/linux/if_link.h describes rx_packets in rtnl_link_stats64
as including "packets which host had to drop at various stages of
processing (even in the driver)".

Take a hardware-good first descriptor that is dropped only because XDP is
enabled and EOP is not set. It jumps to xdp_done and skips the new
total_bytes/total_packets block, so only rx_dropped and work_done are
incremented. igc_clean_rx_irq_zc() does the same for !eop frames.
igc_update_stats() then exports the ring totals directly as
net_stats->rx_packets and rx_bytes.

Frames that the XDP program drops are still counted in the same function,
and before this patch every descriptor was counted. The commit message
says the exclusion is intentional ("without counting successful RX"), but
that doesn't match the documented semantics.

>  		truesize = igc_get_rx_frame_truesize(rx_ring, size);
>  
>  		pktbuf = page_address(rx_buffer->page) + rx_buffer->page_offset;

[ ... ]

> -		if (igc_fpe_is_pmac_enabled(adapter) &&
> +		if (first && igc_fpe_is_pmac_enabled(adapter) &&
>  		    igc_fpe_handle_mpacket(adapter, rx_desc, size, pktbuf + pkt_offset)) {
>  			/* Advance the ring next-to-clean */
>  			igc_is_non_eop(rx_ring, rx_desc);
>  			cleaned_count++;
> +			work_done++;
>  			continue;
>  		}

[Severity: High]
This is a pre-existing issue, but this block is being modified here. Does
this early exit leak the RX page and its DMA mapping?

igc_get_rx_buffer() always decrements rx_buffer->pagecnt_bias. This path
advances next_to_clean and continues, but it never restores the bias and
never calls igc_put_rx_buffer(). That function is the only place that
recycles or unmaps the page and clears rx_buffer->page.

The slot still holds the page after next_to_clean has moved past it. If
igc_reuse_rx_page() later recycles a buffer into that slot, it overwrites
the page without checking:

drivers/net/ethernet/intel/igc/igc_main.c:igc_reuse_rx_page() {
    ...
	new_buff = &rx_ring->rx_buffer_info[nta];
    ...
	new_buff->page		= old_buff->page;
    ...
}

The old page and its mapping are then lost.

If igc_alloc_rx_buffers() reaches the slot first, igc_alloc_mapped_page()
reuses the page because bi->page is non-NULL, but the bias is one too
low. igc_can_reuse_rx_page() would then always see
(rx_buffer_pgcnt - pagecnt_bias) > 1, so the page is never freed.

igc_fpe_handle_mpacket() returns true for any SMD-V or SMD-R frame. With
pMAC enabled, the link partner can trigger this with every such frame it
sends.

The new drop path in this patch already releases the buffer correctly:
pagecnt_bias++ in the IGC_XDP_CONSUMED case, then igc_put_rx_buffer(),
cleaned_count++ and igc_is_non_eop(). Could the mPacket case release the
buffer the same way?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261025065154.2037149-1-p%401g4.org

Re: [PATCH net] igc: track rx frame boundaries independent of skbs

[Paul Moses]

On Monday, October 26th, 2026 at 2:43 AM, netdev-bot+sashiko@kernel.org <netdev-bot+sashiko@kernel.org> wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (2) · Medium: 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).
> 
> - [Medium] The new split-frame discard paths leave frames out of
>   rx_packets and rx_bytes, which contradicts the rtnl_link_stats64…
> 
> Pre-existing issues:
> - [High] In igc_clean_rx_irq(), the FPE mPacket early exit leaks the RX
>   page and its DMA mapping.
> - [High] igc_xdp_set_prog(), igc_xdp_enable_pool() and
>   igc_xdp_disable_pool() tear down and rebuild the RX ring while NAPI…
> 
> --
> 
> igc: track rx frame boundaries independent of skbs
> 
> The patch adds a per-ring rx_in_frame flag so the inline TSIP timestamp
> header is stripped only from the first buffer of a frame. Split frames
> that are not supported (XDP enabled, AF_XDP zero-copy) are rejected
> before XDP runs, and their continuation descriptors are drained. The RX
> poll functions now return a work_done count that includes discarded
> descriptors, instead of total_packets.
> 
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> > index 1fb5f3cbe93c5..9ca5558c762c9 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -474,6 +474,7 @@ static void igc_clean_rx_ring(struct igc_ring *ring)
> >  		igc_clean_rx_ring_page_shared(ring);
> >
> >  	clear_ring_uses_large_buffer(ring);
> > +	ring->rx_in_frame = false;
> >
> >  	ring->next_to_alloc = 0;
> >  	ring->next_to_clean = 0;
> > @@ -673,6 +674,7 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
> >  	/* reset next-to- use/clean to place SW in sync with hardware */
> >  	ring->next_to_clean = 0;
> >  	ring->next_to_use = 0;
> > +	ring->rx_in_frame = false;
> >
> >  	if (ring->xsk_pool)
> >  		buf_size = xsk_pool_get_rx_frame_size(ring->xsk_pool);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but these new resets assume
> NAPI is not polling the ring. Do the XDP reconfiguration paths actually
> guarantee that?
> 
> igc_xdp_set_prog(), igc_xdp_enable_pool() and igc_xdp_disable_pool() all
> clean the ring before they disable NAPI:
> 
> drivers/net/ethernet/intel/igc/igc_xdp.c:igc_xdp_set_prog() {
>     ...
> 		for (i = 0; i < adapter->num_rx_queues; i++) {
> 			igc_disable_rx_ring(adapter->rx_ring[i]);
> 			igc_disable_tx_ring(adapter->tx_ring[i]);
> 			napi_disable(&adapter->rx_ring[i]->q_vector->napi);
> 		}
>     ...
> 			napi_enable(&adapter->rx_ring[i]->q_vector->napi);
> 			igc_enable_tx_ring(adapter->tx_ring[i]);
> 			igc_enable_rx_ring(adapter->rx_ring[i]);
>     ...
> }
> 
> igc_disable_rx_ring() does only an RXDCTL write before it calls
> igc_clean_rx_ring(). Also, napi_enable() runs before
> igc_enable_rx_ring()->igc_configure_rx_ring().
> 
> Suppose igc_clean_rx_irq() is running on another CPU while
> igc_clean_rx_ring_page_shared() calls dev_kfree_skb(rx_ring->skb) and
> drains the pages. Could the poll then call igc_add_rx_frag() on the freed
> skb, or drop page references a second time through igc_put_rx_buffer()?
> 
> When the poll exits, it also writes rx_ring->skb, next_to_clean and
> rx_in_frame back, after the reset. igc_configure_rx_ring() resets
> rx_in_frame but not rx_ring->skb. A later poll could then see
> first == true with a stale non-NULL skb, and the new (!first && !skb)
> check would not catch that.
> 
> Triggering this needs a CAP_NET_ADMIN user to attach or detach an XDP
> program, or to bind or unbind an AF_XDP pool while XDP is loaded, while
> RX traffic keeps a poll running on another CPU.
> 
> [ ... ]
> 
> > @@ -2635,21 +2635,34 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  		dma_rmb();
> >
> >  		rx_buffer = igc_get_rx_buffer(rx_ring, size, &rx_buffer_pgcnt);
> > +		tsip = first && igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP);
> > +
> > +		if ((!first && !skb) ||
> > +		    (first && igc_xdp_is_enabled(adapter) &&
> > +		     !igc_test_staterr(rx_desc, IGC_RXD_STAT_EOP)) ||
> > +		    unlikely(tsip && size < IGC_TS_HDR_LEN)) {
> > +			if (first)
> > +				dev_core_stats_rx_dropped_inc(rx_ring->netdev);
> > +			xdp_res = IGC_XDP_CONSUMED;
> > +			goto xdp_done;
> > +		}
> > +
> 
> [Severity: Medium]
> Should the frames dropped here still count toward rx_packets and
> rx_bytes?
> 
> include/uapi/linux/if_link.h describes rx_packets in rtnl_link_stats64
> as including "packets which host had to drop at various stages of
> processing (even in the driver)".
> 
> Take a hardware-good first descriptor that is dropped only because XDP is
> enabled and EOP is not set. It jumps to xdp_done and skips the new
> total_bytes/total_packets block, so only rx_dropped and work_done are
> incremented. igc_clean_rx_irq_zc() does the same for !eop frames.
> igc_update_stats() then exports the ring totals directly as
> net_stats->rx_packets and rx_bytes.
> 
> Frames that the XDP program drops are still counted in the same function,
> and before this patch every descriptor was counted. The commit message
> says the exclusion is intentional ("without counting successful RX"), but
> that doesn't match the documented semantics.
> 
> >  		truesize = igc_get_rx_frame_truesize(rx_ring, size);
> >
> >  		pktbuf = page_address(rx_buffer->page) + rx_buffer->page_offset;
> 
> [ ... ]
> 
> > -		if (igc_fpe_is_pmac_enabled(adapter) &&
> > +		if (first && igc_fpe_is_pmac_enabled(adapter) &&
> >  		    igc_fpe_handle_mpacket(adapter, rx_desc, size, pktbuf + pkt_offset)) {
> >  			/* Advance the ring next-to-clean */
> >  			igc_is_non_eop(rx_ring, rx_desc);
> >  			cleaned_count++;
> > +			work_done++;
> >  			continue;
> >  		}
> 
> [Severity: High]
> This is a pre-existing issue, but this block is being modified here. Does
> this early exit leak the RX page and its DMA mapping?
> 
> igc_get_rx_buffer() always decrements rx_buffer->pagecnt_bias. This path
> advances next_to_clean and continues, but it never restores the bias and
> never calls igc_put_rx_buffer(). That function is the only place that
> recycles or unmaps the page and clears rx_buffer->page.
> 
> The slot still holds the page after next_to_clean has moved past it. If
> igc_reuse_rx_page() later recycles a buffer into that slot, it overwrites
> the page without checking:
> 
> drivers/net/ethernet/intel/igc/igc_main.c:igc_reuse_rx_page() {
>     ...
> 	new_buff = &rx_ring->rx_buffer_info[nta];
>     ...
> 	new_buff->page		= old_buff->page;
>     ...
> }
> 
> The old page and its mapping are then lost.
> 
> If igc_alloc_rx_buffers() reaches the slot first, igc_alloc_mapped_page()
> reuses the page because bi->page is non-NULL, but the bias is one too
> low. igc_can_reuse_rx_page() would then always see
> (rx_buffer_pgcnt - pagecnt_bias) > 1, so the page is never freed.
> 
> igc_fpe_handle_mpacket() returns true for any SMD-V or SMD-R frame. With
> pMAC enabled, the link partner can trigger this with every such frame it
> sends.
> 
> The new drop path in this patch already releases the buffer correctly:
> pagecnt_bias++ in the IGC_XDP_CONSUMED case, then igc_put_rx_buffer(),
> cleaned_count++ and igc_is_non_eop(). Could the mPacket case release the
> buffer the same way?
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261025065154.2037149-1-p%401g4.org
> 

I'll review and correct accounting. The other two are out of scope
unless I introduced regression. 

pw-bot: cr

Re: [PATCH net] igc: track rx frame boundaries independent of skbs

[Maciej Fijalkowski]

On Sun, Oct 25, 2026 at 06:52:06AM +0000, Paul Moses wrote:
> TSIP remains set on continuation descriptors, but the inline timestamp
> header exists only in the first buffer. Stripping it from every
> descriptor truncates split frames and can underflow short buffers.
> 
> skb ownership cannot identify frame boundaries when XDP consumes the
> first buffer or AF_XDP uses zero-copy.
> 
> Track rx_in_frame per ring and update it as descriptors are consumed.
> Strip timestamps only from first buffers, reject unsupported split
> frames before XDP and drain continuations. Account discarded
> descriptors against NAPI work without counting successful RX.
> 
> Fixes: e1ed4f92a625 ("igc: Refactor Rx timestamp handling")
> Fixes: 26575105d6ed ("igc: Add initial XDP support")
> Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy")
> Cc: stable@vger.kernel.org
> Signed-off-by: Paul Moses <p@1g4.org>
> ---
> - This supersedes "igc: only strip RX timestamp header from first buffer"
>   following feedback from Simon Horman, Maciej Fijalkowski and Sashiko.
> 
> Changes:
> - track frame boundaries independently of skb ownership
> - guard timestamp stripping and drain unsupported split frames
> - cover the AF_XDP zero-copy RX path
> ---
>  drivers/net/ethernet/intel/igc/igc.h      |  1 +
>  drivers/net/ethernet/intel/igc/igc_main.c | 87 ++++++++++++++++-------
>  2 files changed, 64 insertions(+), 24 deletions(-)

Hi Paul,

I think this implementation went off track. We should make sure we *never*
enter data path for XDP/AF_XDP multi-buffer scenario until we add real
support to it within this driver. Now you sorta handle fragmented packets
in data path.

I tried to argue with bot reports that it hallucinated for
igc_clean_rx_irq() content (which was concise and to the point) and agreed
the AF_XDP should *not* be exposed to any fragmented traffic, which should
be addressed on HW RX ring configuration side, not within data path.

Also this should have been a v3 with a link to previous revisions so we
could keep track what has been changing between versions.

Here's something that would address ZC fragmentation issue. Could you give
it a spin on your side as I don't currently have igc in my setup handy.


From a3edec7db135f13a567565a652da5cc850505f7f Mon Sep 17 00:00:00 2001
From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Date: Mon, 26 Oct 2026 15:00:00 +0100
Subject: [PATCH net] igc: fix AF_XDP receive buffer size for 2K UMEM chunks

SRRCTL.BSIZEPKT has a granularity of 1 KiB. With a 2 KiB UMEM
chunk and the default XDP headroom, xsk_pool_get_rx_frame_size()
returns 1792. Programming this value truncates the hardware receive
buffer size to 1 KiB, even though an MTU-sized frame fits in the
usable UMEM area.

XDP on igc is limited to MTU 1500, so configure a 2 KiB receive
buffer for XSK rings. Limit RLPML to the maximum supported Ethernet
frame size while XDP is attached, regardless of PAGE_SIZE, to avoid
oversized frames spanning descriptors or exceeding the usable UMEM
space.

RLPML is currently programmed from igc_set_rx_mode(), which can run
without RTNL, for example on an IPv6 multicast join, and could then
use a stale XDP state. RLPML does not depend on the address lists.
Program it from igc_setup_rctl() instead, next to RCTL.LPE, and from
igc_xdp_set_prog() while the rings are disabled. This avoids
netif_addr_lock_bh() usage.

Validate that the XSK frame size accommodates a maximum-size frame
with two VLAN tags, FCS and the optional inline RX timestamp. Check
against the maximum supported XDP MTU, rather than the current MTU,
which can change after the socket is bound.

Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h      |  1 +
 drivers/net/ethernet/intel/igc/igc_main.c | 33 +++++++++++++++++------
 drivers/net/ethernet/intel/igc/igc_xdp.c  | 11 +++++---
 3 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 17f213cc93e4..ad1062f8531a 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -765,6 +765,7 @@ static inline s32 igc_read_phy_reg(struct igc_hw *hw, u32 offset, u16 *data)
 	return -EOPNOTSUPP;
 }
 
+void igc_set_rlpml(struct igc_adapter *adapter);
 void igc_reinit_locked(struct igc_adapter *);
 struct igc_nfc_rule *igc_get_nfc_rule(struct igc_adapter *adapter,
 				      u32 location);
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 1fb5f3cbe93c..38a5702647f4 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -674,8 +674,11 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
 	ring->next_to_clean = 0;
 	ring->next_to_use = 0;
 
+	/* igc's XDP limits MTU to 1500; all XSK RX buffers can use 2kb;
+	 * RLPML limits the receive length to fit after UMEM headroom.
+	 */
 	if (ring->xsk_pool)
-		buf_size = xsk_pool_get_rx_frame_size(ring->xsk_pool);
+		buf_size = IGC_RXBUFFER_2048;
 	else if (ring_uses_large_buffer(ring))
 		buf_size = IGC_RXBUFFER_3072;
 	else
@@ -830,6 +833,25 @@ static void igc_setup_mrqc(struct igc_adapter *adapter)
 	wr32(IGC_MRQC, mrqc);
 }
 
+void igc_set_rlpml(struct igc_adapter *adapter)
+{
+	struct igc_hw *hw = &adapter->hw;
+	u32 rlpml = MAX_JUMBO_FRAME_SIZE;
+
+#if (PAGE_SIZE < 8192)
+	if (adapter->max_frame_size <= IGC_MAX_FRAME_BUILD_SKB)
+		rlpml = IGC_MAX_FRAME_BUILD_SKB;
+#endif
+	/* igc's XDP currently does not support jumbo frames. Limit the
+	 * accepted packet size regardless of PAGE_SIZE so it fits in a 2kb
+	 * XSK chunk.
+	 */
+	if (igc_xdp_is_enabled(adapter))
+		rlpml = ETH_FRAME_LEN + VLAN_HLEN * 2 + ETH_FCS_LEN;
+
+	wr32(IGC_RLPML, rlpml);
+}
+
 /**
  * igc_setup_rctl - configure the receive control registers
  * @adapter: Board private structure
@@ -874,6 +896,7 @@ static void igc_setup_rctl(struct igc_adapter *adapter)
 			  IGC_RCTL_CFIEN); /* Disable VLAN CFIEN Filter */
 	}
 
+	igc_set_rlpml(adapter);
 	wr32(IGC_RCTL, rctl);
 }
 
@@ -4116,7 +4139,7 @@ static void igc_set_rx_mode(struct net_device *netdev)
 {
 	struct igc_adapter *adapter = netdev_priv(netdev);
 	struct igc_hw *hw = &adapter->hw;
-	u32 rctl = 0, rlpml = MAX_JUMBO_FRAME_SIZE;
+	u32 rctl = 0;
 	int count;
 
 	/* Check for Promiscuous and All Multicast modes */
@@ -4146,12 +4169,6 @@ static void igc_set_rx_mode(struct net_device *netdev)
 	/* update state of unicast and multicast */
 	rctl |= rd32(IGC_RCTL) & ~(IGC_RCTL_UPE | IGC_RCTL_MPE);
 	wr32(IGC_RCTL, rctl);
-
-#if (PAGE_SIZE < 8192)
-	if (adapter->max_frame_size <= IGC_MAX_FRAME_BUILD_SKB)
-		rlpml = IGC_MAX_FRAME_BUILD_SKB;
-#endif
-	wr32(IGC_RLPML, rlpml);
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/igc/igc_xdp.c b/drivers/net/ethernet/intel/igc/igc_xdp.c
index 9eb47b4beb06..a0f1ecb2ed5f 100644
--- a/drivers/net/ethernet/intel/igc/igc_xdp.c
+++ b/drivers/net/ethernet/intel/igc/igc_xdp.c
@@ -34,6 +34,8 @@ int igc_xdp_set_prog(struct igc_adapter *adapter, struct bpf_prog *prog,
 	}
 
 	old_prog = xchg(&adapter->xdp_prog, prog);
+	if (if_running && need_update)
+		igc_set_rlpml(adapter);
 	if (old_prog)
 		bpf_prog_put(old_prog);
 
@@ -69,11 +71,12 @@ static int igc_xdp_enable_pool(struct igc_adapter *adapter,
 		return -EINVAL;
 
 	frame_size = xsk_pool_get_rx_frame_size(pool);
-	if (frame_size < ETH_FRAME_LEN + VLAN_HLEN * 2) {
+	if (frame_size < ETH_FRAME_LEN + VLAN_HLEN * 2 +
+			 ETH_FCS_LEN + IGC_TS_HDR_LEN) {
 		/* When XDP is enabled, the driver doesn't support frames that
-		 * span over multiple buffers. To avoid that, we check if xsk
-		 * frame size is big enough to fit the max ethernet frame size
-		 * + vlan double tagging.
+		 * span multiple buffers. Check the maximum supported MTU, not
+		 * current MTU, as it may change while the pool is bound. Account
+		 * for double VLAN tags, FCS and inline RX timestamping.
 		 */
 		return -EOPNOTSUPP;
 	}
-- 
2.43.0


> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index 17f213cc93e4..635d49701e62 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -198,6 +198,7 @@ struct igc_ring {
>  			struct igc_rx_packet_stats pkt_stats;
>  			struct u64_stats_sync rx_syncp;
>  			struct sk_buff *skb;
> +			bool rx_in_frame;
>  		};
>  	};
>  
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 1fb5f3cbe93c..9ca5558c762c 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -474,6 +474,7 @@ static void igc_clean_rx_ring(struct igc_ring *ring)
>  		igc_clean_rx_ring_page_shared(ring);
>  
>  	clear_ring_uses_large_buffer(ring);
> +	ring->rx_in_frame = false;
>  
>  	ring->next_to_alloc = 0;
>  	ring->next_to_clean = 0;
> @@ -673,6 +674,7 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
>  	/* reset next-to- use/clean to place SW in sync with hardware */
>  	ring->next_to_clean = 0;
>  	ring->next_to_use = 0;
> +	ring->rx_in_frame = false;
>  
>  	if (ring->xsk_pool)
>  		buf_size = xsk_pool_get_rx_frame_size(ring->xsk_pool);
> @@ -2111,10 +2113,8 @@ static bool igc_can_reuse_rx_page(struct igc_rx_buffer *rx_buffer,
>   * @rx_ring: Rx ring being processed
>   * @rx_desc: Rx descriptor for current buffer
>   *
> - * This function updates next to clean.  If the buffer is an EOP buffer
> - * this function exits returning false, otherwise it will place the
> - * sk_buff in the next buffer to be chained and return true indicating
> - * that this is in fact a non-EOP buffer.
> + * Advance next to clean and retain frame position for the next descriptor.
> + * Return true if the consumed descriptor does not end the frame.
>   */
>  static bool igc_is_non_eop(struct igc_ring *rx_ring,
>  			   union igc_adv_rx_desc *rx_desc)
> @@ -2127,10 +2127,8 @@ static bool igc_is_non_eop(struct igc_ring *rx_ring,
>  
>  	prefetch(IGC_RX_DESC(rx_ring, ntc));
>  
> -	if (likely(igc_test_staterr(rx_desc, IGC_RXD_STAT_EOP)))
> -		return false;
> -
> -	return true;
> +	rx_ring->rx_in_frame = !igc_test_staterr(rx_desc, IGC_RXD_STAT_EOP);
> +	return unlikely(rx_ring->rx_in_frame);
>  }
>  
>  /**
> @@ -2601,7 +2599,7 @@ static void igc_update_rx_stats(struct igc_q_vector *q_vector,
>  
>  static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>  {
> -	unsigned int total_bytes = 0, total_packets = 0;
> +	unsigned int total_bytes = 0, total_packets = 0, work_done = 0;
>  	struct igc_adapter *adapter = q_vector->adapter;
>  	struct igc_ring *rx_ring = q_vector->rx.ring;
>  	struct sk_buff *skb = rx_ring->skb;
> @@ -2609,13 +2607,15 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>  	int xdp_status = 0, rx_buffer_pgcnt;
>  	int xdp_res = 0;
>  
> -	while (likely(total_packets < budget)) {
> +	while (likely(work_done < budget)) {
>  		struct igc_xdp_buff ctx = { .rx_ts = NULL };
> +		bool first = !rx_ring->rx_in_frame;
>  		struct igc_rx_buffer *rx_buffer;
>  		union igc_adv_rx_desc *rx_desc;
>  		unsigned int size, truesize;
>  		int pkt_offset = 0;
>  		void *pktbuf;
> +		bool tsip;
>  
>  		/* return some buffers to hardware, one at a time is too slow */
>  		if (cleaned_count >= IGC_RX_BUFFER_WRITE) {
> @@ -2635,21 +2635,34 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>  		dma_rmb();
>  
>  		rx_buffer = igc_get_rx_buffer(rx_ring, size, &rx_buffer_pgcnt);
> +		tsip = first && igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP);
> +
> +		if ((!first && !skb) ||
> +		    (first && igc_xdp_is_enabled(adapter) &&
> +		     !igc_test_staterr(rx_desc, IGC_RXD_STAT_EOP)) ||
> +		    unlikely(tsip && size < IGC_TS_HDR_LEN)) {
> +			if (first)
> +				dev_core_stats_rx_dropped_inc(rx_ring->netdev);
> +			xdp_res = IGC_XDP_CONSUMED;
> +			goto xdp_done;
> +		}
> +
>  		truesize = igc_get_rx_frame_truesize(rx_ring, size);
>  
>  		pktbuf = page_address(rx_buffer->page) + rx_buffer->page_offset;
>  
> -		if (igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)) {
> +		if (tsip) {
>  			ctx.rx_ts = pktbuf;
>  			pkt_offset = IGC_TS_HDR_LEN;
>  			size -= IGC_TS_HDR_LEN;
>  		}
>  
> -		if (igc_fpe_is_pmac_enabled(adapter) &&
> +		if (first && igc_fpe_is_pmac_enabled(adapter) &&
>  		    igc_fpe_handle_mpacket(adapter, rx_desc, size, pktbuf + pkt_offset)) {
>  			/* Advance the ring next-to-clean */
>  			igc_is_non_eop(rx_ring, rx_desc);
>  			cleaned_count++;
> +			work_done++;
>  			continue;
>  		}
>  
> @@ -2664,6 +2677,12 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>  			xdp_res = igc_xdp_run_prog(adapter, &ctx.xdp);
>  		}
>  
> +		if (xdp_res) {
> +			total_bytes += size;
> +			total_packets++;
> +		}
> +
> +xdp_done:
>  		if (xdp_res) {
>  			switch (xdp_res) {
>  			case IGC_XDP_CONSUMED:
> @@ -2676,8 +2695,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>  				break;
>  			}
>  
> -			total_packets++;
> -			total_bytes += size;
> +			work_done++;
>  		} else if (skb)
>  			igc_add_rx_frag(rx_ring, rx_buffer, skb, size);
>  		else if (ring_uses_build_skb(rx_ring))
> @@ -2700,8 +2718,15 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>  		if (igc_is_non_eop(rx_ring, rx_desc))
>  			continue;
>  
> +		if (xdp_res) {
> +			skb = NULL;
> +			continue;
> +		}
> +
> +		work_done++;
> +
>  		/* verify the packet layout is correct */
> -		if (xdp_res || igc_cleanup_headers(rx_ring, rx_desc, skb)) {
> +		if (igc_cleanup_headers(rx_ring, rx_desc, skb)) {
>  			skb = NULL;
>  			continue;
>  		}
> @@ -2717,7 +2742,6 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>  		/* reset skb pointer */
>  		skb = NULL;
>  
> -		/* update budget accounting */
>  		total_packets++;
>  	}
>  
> @@ -2732,7 +2756,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>  	if (cleaned_count)
>  		igc_alloc_rx_buffers(rx_ring, cleaned_count);
>  
> -	return total_packets;
> +	return work_done;
>  }
>  
>  static struct sk_buff *igc_construct_skb_zc(struct igc_ring *ring,
> @@ -2797,10 +2821,10 @@ static struct igc_xdp_buff *xsk_buff_to_igc_ctx(struct xdp_buff *xdp)
>  
>  static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
>  {
> +	int total_bytes = 0, total_packets = 0, work_done = 0;
>  	struct igc_adapter *adapter = q_vector->adapter;
>  	struct igc_ring *ring = q_vector->rx.ring;
>  	u16 cleaned_count = igc_desc_unused(ring);
> -	int total_bytes = 0, total_packets = 0;
>  	u16 ntc = ring->next_to_clean;
>  	struct bpf_prog *prog;
>  	bool failure = false;
> @@ -2810,11 +2834,13 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
>  
>  	prog = READ_ONCE(adapter->xdp_prog);
>  
> -	while (likely(total_packets < budget)) {
> +	while (likely(work_done < budget)) {
> +		bool first = !ring->rx_in_frame;
>  		union igc_adv_rx_desc *desc;
>  		struct igc_rx_buffer *bi;
>  		struct igc_xdp_buff *ctx;
>  		unsigned int size;
> +		bool eop, tsip;
>  		int res;
>  
>  		desc = IGC_RX_DESC(ring, ntc);
> @@ -2829,11 +2855,20 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
>  		dma_rmb();
>  
>  		bi = &ring->rx_buffer_info[ntc];
> +		tsip = first && igc_test_staterr(desc, IGC_RXDADV_STAT_TSIP);
> +		eop = igc_test_staterr(desc, IGC_RXD_STAT_EOP);
> +
> +		if (!first || !eop || unlikely(tsip && size < IGC_TS_HDR_LEN)) {
> +			if (first)
> +				dev_core_stats_rx_dropped_inc(ring->netdev);
> +			res = IGC_XDP_CONSUMED;
> +			goto xdp_done;
> +		}
>  
>  		ctx = xsk_buff_to_igc_ctx(bi->xdp);
>  		ctx->rx_desc = desc;
>  
> -		if (igc_test_staterr(desc, IGC_RXDADV_STAT_TSIP)) {
> +		if (tsip) {
>  			ctx->rx_ts = bi->xdp->data;
>  
>  			bi->xdp->data += IGC_TS_HDR_LEN;
> @@ -2851,6 +2886,10 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
>  		xsk_buff_dma_sync_for_cpu(bi->xdp);
>  
>  		res = __igc_xdp_run_prog(adapter, prog, bi->xdp);
> +		total_bytes += size;
> +		total_packets++;
> +
> +xdp_done:
>  		switch (res) {
>  		case IGC_XDP_PASS:
>  			igc_dispatch_skb_zc(q_vector, desc, ctx);
> @@ -2865,8 +2904,8 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
>  		}
>  
>  		bi->xdp = NULL;
> -		total_bytes += size;
> -		total_packets++;
> +		ring->rx_in_frame = !eop;
> +		work_done++;
>  		cleaned_count++;
>  		ntc++;
>  		if (ntc == ring->count)
> @@ -2889,10 +2928,10 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
>  			xsk_set_rx_need_wakeup(ring->xsk_pool);
>  		else
>  			xsk_clear_rx_need_wakeup(ring->xsk_pool);
> -		return total_packets;
> +		return work_done;
>  	}
>  
> -	return failure ? budget : total_packets;
> +	return failure ? budget : work_done;
>  }
>  
>  static void igc_update_tx_stats(struct igc_q_vector *q_vector,
> 

Re: [PATCH net] igc: track rx frame boundaries independent of skbs

[Paul Moses]

On Monday, October 26th, 2026 at 2:46 PM, Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote:

> On Sun, Oct 25, 2026 at 06:52:06AM +0000, Paul Moses wrote:
> > TSIP remains set on continuation descriptors, but the inline timestamp
> > header exists only in the first buffer. Stripping it from every
> > descriptor truncates split frames and can underflow short buffers.
> >
> > skb ownership cannot identify frame boundaries when XDP consumes the
> > first buffer or AF_XDP uses zero-copy.
> >
> > Track rx_in_frame per ring and update it as descriptors are consumed.
> > Strip timestamps only from first buffers, reject unsupported split
> > frames before XDP and drain continuations. Account discarded
> > descriptors against NAPI work without counting successful RX.
> >
> > Fixes: e1ed4f92a625 ("igc: Refactor Rx timestamp handling")
> > Fixes: 26575105d6ed ("igc: Add initial XDP support")
> > Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Paul Moses <p@1g4.org>
> > ---
> > - This supersedes "igc: only strip RX timestamp header from first buffer"
> >   following feedback from Simon Horman, Maciej Fijalkowski and Sashiko.
> >
> > Changes:
> > - track frame boundaries independently of skb ownership
> > - guard timestamp stripping and drain unsupported split frames
> > - cover the AF_XDP zero-copy RX path
> > ---
> >  drivers/net/ethernet/intel/igc/igc.h      |  1 +
> >  drivers/net/ethernet/intel/igc/igc_main.c | 87 ++++++++++++++++-------
> >  2 files changed, 64 insertions(+), 24 deletions(-)
> 
> Hi Paul,
> 
> I think this implementation went off track. We should make sure we *never*
> enter data path for XDP/AF_XDP multi-buffer scenario until we add real
> support to it within this driver. Now you sorta handle fragmented packets
> in data path.
> 
> I tried to argue with bot reports that it hallucinated for
> igc_clean_rx_irq() content (which was concise and to the point) and agreed
> the AF_XDP should *not* be exposed to any fragmented traffic, which should
> be addressed on HW RX ring configuration side, not within data path.
> 
> Also this should have been a v3 with a link to previous revisions so we
> could keep track what has been changing between versions.
> 
> Here's something that would address ZC fragmentation issue. Could you give
> it a spin on your side as I don't currently have igc in my setup handy.
> 
> 
> From a3edec7db135f13a567565a652da5cc850505f7f Mon Sep 17 00:00:00 2001
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Date: Mon, 26 Oct 2026 15:00:00 +0100
> Subject: [PATCH net] igc: fix AF_XDP receive buffer size for 2K UMEM chunks
> 
> SRRCTL.BSIZEPKT has a granularity of 1 KiB. With a 2 KiB UMEM
> chunk and the default XDP headroom, xsk_pool_get_rx_frame_size()
> returns 1792. Programming this value truncates the hardware receive
> buffer size to 1 KiB, even though an MTU-sized frame fits in the
> usable UMEM area.
> 
> XDP on igc is limited to MTU 1500, so configure a 2 KiB receive
> buffer for XSK rings. Limit RLPML to the maximum supported Ethernet
> frame size while XDP is attached, regardless of PAGE_SIZE, to avoid
> oversized frames spanning descriptors or exceeding the usable UMEM
> space.
> 
> RLPML is currently programmed from igc_set_rx_mode(), which can run
> without RTNL, for example on an IPv6 multicast join, and could then
> use a stale XDP state. RLPML does not depend on the address lists.
> Program it from igc_setup_rctl() instead, next to RCTL.LPE, and from
> igc_xdp_set_prog() while the rings are disabled. This avoids
> netif_addr_lock_bh() usage.
> 
> Validate that the XSK frame size accommodates a maximum-size frame
> with two VLAN tags, FCS and the optional inline RX timestamp. Check
> against the maximum supported XDP MTU, rather than the current MTU,
> which can change after the socket is bound.
> 
> Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy")
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc.h      |  1 +
>  drivers/net/ethernet/intel/igc/igc_main.c | 33 +++++++++++++++++------
>  drivers/net/ethernet/intel/igc/igc_xdp.c  | 11 +++++---
>  3 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index 17f213cc93e4..ad1062f8531a 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -765,6 +765,7 @@ static inline s32 igc_read_phy_reg(struct igc_hw *hw, u32 offset, u16 *data)
>  	return -EOPNOTSUPP;
>  }
> 
> +void igc_set_rlpml(struct igc_adapter *adapter);
>  void igc_reinit_locked(struct igc_adapter *);
>  struct igc_nfc_rule *igc_get_nfc_rule(struct igc_adapter *adapter,
>  				      u32 location);
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 1fb5f3cbe93c..38a5702647f4 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -674,8 +674,11 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
>  	ring->next_to_clean = 0;
>  	ring->next_to_use = 0;
> 
> +	/* igc's XDP limits MTU to 1500; all XSK RX buffers can use 2kb;
> +	 * RLPML limits the receive length to fit after UMEM headroom.
> +	 */
>  	if (ring->xsk_pool)
> -		buf_size = xsk_pool_get_rx_frame_size(ring->xsk_pool);
> +		buf_size = IGC_RXBUFFER_2048;
>  	else if (ring_uses_large_buffer(ring))
>  		buf_size = IGC_RXBUFFER_3072;
>  	else
> @@ -830,6 +833,25 @@ static void igc_setup_mrqc(struct igc_adapter *adapter)
>  	wr32(IGC_MRQC, mrqc);
>  }
> 
> +void igc_set_rlpml(struct igc_adapter *adapter)
> +{
> +	struct igc_hw *hw = &adapter->hw;
> +	u32 rlpml = MAX_JUMBO_FRAME_SIZE;
> +
> +#if (PAGE_SIZE < 8192)
> +	if (adapter->max_frame_size <= IGC_MAX_FRAME_BUILD_SKB)
> +		rlpml = IGC_MAX_FRAME_BUILD_SKB;
> +#endif
> +	/* igc's XDP currently does not support jumbo frames. Limit the
> +	 * accepted packet size regardless of PAGE_SIZE so it fits in a 2kb
> +	 * XSK chunk.
> +	 */
> +	if (igc_xdp_is_enabled(adapter))
> +		rlpml = ETH_FRAME_LEN + VLAN_HLEN * 2 + ETH_FCS_LEN;
> +
> +	wr32(IGC_RLPML, rlpml);
> +}
> +
>  /**
>   * igc_setup_rctl - configure the receive control registers
>   * @adapter: Board private structure
> @@ -874,6 +896,7 @@ static void igc_setup_rctl(struct igc_adapter *adapter)
>  			  IGC_RCTL_CFIEN); /* Disable VLAN CFIEN Filter */
>  	}
> 
> +	igc_set_rlpml(adapter);
>  	wr32(IGC_RCTL, rctl);
>  }
> 
> @@ -4116,7 +4139,7 @@ static void igc_set_rx_mode(struct net_device *netdev)
>  {
>  	struct igc_adapter *adapter = netdev_priv(netdev);
>  	struct igc_hw *hw = &adapter->hw;
> -	u32 rctl = 0, rlpml = MAX_JUMBO_FRAME_SIZE;
> +	u32 rctl = 0;
>  	int count;
> 
>  	/* Check for Promiscuous and All Multicast modes */
> @@ -4146,12 +4169,6 @@ static void igc_set_rx_mode(struct net_device *netdev)
>  	/* update state of unicast and multicast */
>  	rctl |= rd32(IGC_RCTL) & ~(IGC_RCTL_UPE | IGC_RCTL_MPE);
>  	wr32(IGC_RCTL, rctl);
> -
> -#if (PAGE_SIZE < 8192)
> -	if (adapter->max_frame_size <= IGC_MAX_FRAME_BUILD_SKB)
> -		rlpml = IGC_MAX_FRAME_BUILD_SKB;
> -#endif
> -	wr32(IGC_RLPML, rlpml);
>  }
> 
>  /**
> diff --git a/drivers/net/ethernet/intel/igc/igc_xdp.c b/drivers/net/ethernet/intel/igc/igc_xdp.c
> index 9eb47b4beb06..a0f1ecb2ed5f 100644
> --- a/drivers/net/ethernet/intel/igc/igc_xdp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_xdp.c
> @@ -34,6 +34,8 @@ int igc_xdp_set_prog(struct igc_adapter *adapter, struct bpf_prog *prog,
>  	}
> 
>  	old_prog = xchg(&adapter->xdp_prog, prog);
> +	if (if_running && need_update)
> +		igc_set_rlpml(adapter);
>  	if (old_prog)
>  		bpf_prog_put(old_prog);
> 
> @@ -69,11 +71,12 @@ static int igc_xdp_enable_pool(struct igc_adapter *adapter,
>  		return -EINVAL;
> 
>  	frame_size = xsk_pool_get_rx_frame_size(pool);
> -	if (frame_size < ETH_FRAME_LEN + VLAN_HLEN * 2) {
> +	if (frame_size < ETH_FRAME_LEN + VLAN_HLEN * 2 +
> +			 ETH_FCS_LEN + IGC_TS_HDR_LEN) {
>  		/* When XDP is enabled, the driver doesn't support frames that
> -		 * span over multiple buffers. To avoid that, we check if xsk
> -		 * frame size is big enough to fit the max ethernet frame size
> -		 * + vlan double tagging.
> +		 * span multiple buffers. Check the maximum supported MTU, not
> +		 * current MTU, as it may change while the pool is bound. Account
> +		 * for double VLAN tags, FCS and inline RX timestamping.
>  		 */
>  		return -EOPNOTSUPP;
>  	}
> --
> 2.43.0
> 
> 
> >
> > diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> > index 17f213cc93e4..635d49701e62 100644
> > --- a/drivers/net/ethernet/intel/igc/igc.h
> > +++ b/drivers/net/ethernet/intel/igc/igc.h
> > @@ -198,6 +198,7 @@ struct igc_ring {
> >  			struct igc_rx_packet_stats pkt_stats;
> >  			struct u64_stats_sync rx_syncp;
> >  			struct sk_buff *skb;
> > +			bool rx_in_frame;
> >  		};
> >  	};
> >
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> > index 1fb5f3cbe93c..9ca5558c762c 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -474,6 +474,7 @@ static void igc_clean_rx_ring(struct igc_ring *ring)
> >  		igc_clean_rx_ring_page_shared(ring);
> >
> >  	clear_ring_uses_large_buffer(ring);
> > +	ring->rx_in_frame = false;
> >
> >  	ring->next_to_alloc = 0;
> >  	ring->next_to_clean = 0;
> > @@ -673,6 +674,7 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
> >  	/* reset next-to- use/clean to place SW in sync with hardware */
> >  	ring->next_to_clean = 0;
> >  	ring->next_to_use = 0;
> > +	ring->rx_in_frame = false;
> >
> >  	if (ring->xsk_pool)
> >  		buf_size = xsk_pool_get_rx_frame_size(ring->xsk_pool);
> > @@ -2111,10 +2113,8 @@ static bool igc_can_reuse_rx_page(struct igc_rx_buffer *rx_buffer,
> >   * @rx_ring: Rx ring being processed
> >   * @rx_desc: Rx descriptor for current buffer
> >   *
> > - * This function updates next to clean.  If the buffer is an EOP buffer
> > - * this function exits returning false, otherwise it will place the
> > - * sk_buff in the next buffer to be chained and return true indicating
> > - * that this is in fact a non-EOP buffer.
> > + * Advance next to clean and retain frame position for the next descriptor.
> > + * Return true if the consumed descriptor does not end the frame.
> >   */
> >  static bool igc_is_non_eop(struct igc_ring *rx_ring,
> >  			   union igc_adv_rx_desc *rx_desc)
> > @@ -2127,10 +2127,8 @@ static bool igc_is_non_eop(struct igc_ring *rx_ring,
> >
> >  	prefetch(IGC_RX_DESC(rx_ring, ntc));
> >
> > -	if (likely(igc_test_staterr(rx_desc, IGC_RXD_STAT_EOP)))
> > -		return false;
> > -
> > -	return true;
> > +	rx_ring->rx_in_frame = !igc_test_staterr(rx_desc, IGC_RXD_STAT_EOP);
> > +	return unlikely(rx_ring->rx_in_frame);
> >  }
> >
> >  /**
> > @@ -2601,7 +2599,7 @@ static void igc_update_rx_stats(struct igc_q_vector *q_vector,
> >
> >  static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  {
> > -	unsigned int total_bytes = 0, total_packets = 0;
> > +	unsigned int total_bytes = 0, total_packets = 0, work_done = 0;
> >  	struct igc_adapter *adapter = q_vector->adapter;
> >  	struct igc_ring *rx_ring = q_vector->rx.ring;
> >  	struct sk_buff *skb = rx_ring->skb;
> > @@ -2609,13 +2607,15 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  	int xdp_status = 0, rx_buffer_pgcnt;
> >  	int xdp_res = 0;
> >
> > -	while (likely(total_packets < budget)) {
> > +	while (likely(work_done < budget)) {
> >  		struct igc_xdp_buff ctx = { .rx_ts = NULL };
> > +		bool first = !rx_ring->rx_in_frame;
> >  		struct igc_rx_buffer *rx_buffer;
> >  		union igc_adv_rx_desc *rx_desc;
> >  		unsigned int size, truesize;
> >  		int pkt_offset = 0;
> >  		void *pktbuf;
> > +		bool tsip;
> >
> >  		/* return some buffers to hardware, one at a time is too slow */
> >  		if (cleaned_count >= IGC_RX_BUFFER_WRITE) {
> > @@ -2635,21 +2635,34 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  		dma_rmb();
> >
> >  		rx_buffer = igc_get_rx_buffer(rx_ring, size, &rx_buffer_pgcnt);
> > +		tsip = first && igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP);
> > +
> > +		if ((!first && !skb) ||
> > +		    (first && igc_xdp_is_enabled(adapter) &&
> > +		     !igc_test_staterr(rx_desc, IGC_RXD_STAT_EOP)) ||
> > +		    unlikely(tsip && size < IGC_TS_HDR_LEN)) {
> > +			if (first)
> > +				dev_core_stats_rx_dropped_inc(rx_ring->netdev);
> > +			xdp_res = IGC_XDP_CONSUMED;
> > +			goto xdp_done;
> > +		}
> > +
> >  		truesize = igc_get_rx_frame_truesize(rx_ring, size);
> >
> >  		pktbuf = page_address(rx_buffer->page) + rx_buffer->page_offset;
> >
> > -		if (igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)) {
> > +		if (tsip) {
> >  			ctx.rx_ts = pktbuf;
> >  			pkt_offset = IGC_TS_HDR_LEN;
> >  			size -= IGC_TS_HDR_LEN;
> >  		}
> >
> > -		if (igc_fpe_is_pmac_enabled(adapter) &&
> > +		if (first && igc_fpe_is_pmac_enabled(adapter) &&
> >  		    igc_fpe_handle_mpacket(adapter, rx_desc, size, pktbuf + pkt_offset)) {
> >  			/* Advance the ring next-to-clean */
> >  			igc_is_non_eop(rx_ring, rx_desc);
> >  			cleaned_count++;
> > +			work_done++;
> >  			continue;
> >  		}
> >
> > @@ -2664,6 +2677,12 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  			xdp_res = igc_xdp_run_prog(adapter, &ctx.xdp);
> >  		}
> >
> > +		if (xdp_res) {
> > +			total_bytes += size;
> > +			total_packets++;
> > +		}
> > +
> > +xdp_done:
> >  		if (xdp_res) {
> >  			switch (xdp_res) {
> >  			case IGC_XDP_CONSUMED:
> > @@ -2676,8 +2695,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  				break;
> >  			}
> >
> > -			total_packets++;
> > -			total_bytes += size;
> > +			work_done++;
> >  		} else if (skb)
> >  			igc_add_rx_frag(rx_ring, rx_buffer, skb, size);
> >  		else if (ring_uses_build_skb(rx_ring))
> > @@ -2700,8 +2718,15 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  		if (igc_is_non_eop(rx_ring, rx_desc))
> >  			continue;
> >
> > +		if (xdp_res) {
> > +			skb = NULL;
> > +			continue;
> > +		}
> > +
> > +		work_done++;
> > +
> >  		/* verify the packet layout is correct */
> > -		if (xdp_res || igc_cleanup_headers(rx_ring, rx_desc, skb)) {
> > +		if (igc_cleanup_headers(rx_ring, rx_desc, skb)) {
> >  			skb = NULL;
> >  			continue;
> >  		}
> > @@ -2717,7 +2742,6 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  		/* reset skb pointer */
> >  		skb = NULL;
> >
> > -		/* update budget accounting */
> >  		total_packets++;
> >  	}
> >
> > @@ -2732,7 +2756,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  	if (cleaned_count)
> >  		igc_alloc_rx_buffers(rx_ring, cleaned_count);
> >
> > -	return total_packets;
> > +	return work_done;
> >  }
> >
> >  static struct sk_buff *igc_construct_skb_zc(struct igc_ring *ring,
> > @@ -2797,10 +2821,10 @@ static struct igc_xdp_buff *xsk_buff_to_igc_ctx(struct xdp_buff *xdp)
> >
> >  static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
> >  {
> > +	int total_bytes = 0, total_packets = 0, work_done = 0;
> >  	struct igc_adapter *adapter = q_vector->adapter;
> >  	struct igc_ring *ring = q_vector->rx.ring;
> >  	u16 cleaned_count = igc_desc_unused(ring);
> > -	int total_bytes = 0, total_packets = 0;
> >  	u16 ntc = ring->next_to_clean;
> >  	struct bpf_prog *prog;
> >  	bool failure = false;
> > @@ -2810,11 +2834,13 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
> >
> >  	prog = READ_ONCE(adapter->xdp_prog);
> >
> > -	while (likely(total_packets < budget)) {
> > +	while (likely(work_done < budget)) {
> > +		bool first = !ring->rx_in_frame;
> >  		union igc_adv_rx_desc *desc;
> >  		struct igc_rx_buffer *bi;
> >  		struct igc_xdp_buff *ctx;
> >  		unsigned int size;
> > +		bool eop, tsip;
> >  		int res;
> >
> >  		desc = IGC_RX_DESC(ring, ntc);
> > @@ -2829,11 +2855,20 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
> >  		dma_rmb();
> >
> >  		bi = &ring->rx_buffer_info[ntc];
> > +		tsip = first && igc_test_staterr(desc, IGC_RXDADV_STAT_TSIP);
> > +		eop = igc_test_staterr(desc, IGC_RXD_STAT_EOP);
> > +
> > +		if (!first || !eop || unlikely(tsip && size < IGC_TS_HDR_LEN)) {
> > +			if (first)
> > +				dev_core_stats_rx_dropped_inc(ring->netdev);
> > +			res = IGC_XDP_CONSUMED;
> > +			goto xdp_done;
> > +		}
> >
> >  		ctx = xsk_buff_to_igc_ctx(bi->xdp);
> >  		ctx->rx_desc = desc;
> >
> > -		if (igc_test_staterr(desc, IGC_RXDADV_STAT_TSIP)) {
> > +		if (tsip) {
> >  			ctx->rx_ts = bi->xdp->data;
> >
> >  			bi->xdp->data += IGC_TS_HDR_LEN;
> > @@ -2851,6 +2886,10 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
> >  		xsk_buff_dma_sync_for_cpu(bi->xdp);
> >
> >  		res = __igc_xdp_run_prog(adapter, prog, bi->xdp);
> > +		total_bytes += size;
> > +		total_packets++;
> > +
> > +xdp_done:
> >  		switch (res) {
> >  		case IGC_XDP_PASS:
> >  			igc_dispatch_skb_zc(q_vector, desc, ctx);
> > @@ -2865,8 +2904,8 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
> >  		}
> >
> >  		bi->xdp = NULL;
> > -		total_bytes += size;
> > -		total_packets++;
> > +		ring->rx_in_frame = !eop;
> > +		work_done++;
> >  		cleaned_count++;
> >  		ntc++;
> >  		if (ntc == ring->count)
> > @@ -2889,10 +2928,10 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
> >  			xsk_set_rx_need_wakeup(ring->xsk_pool);
> >  		else
> >  			xsk_clear_rx_need_wakeup(ring->xsk_pool);
> > -		return total_packets;
> > +		return work_done;
> >  	}
> >
> > -	return failure ? budget : total_packets;
> > +	return failure ? budget : work_done;
> >  }
> >
> >  static void igc_update_tx_stats(struct igc_q_vector *q_vector,
> >
> 

Looks good to me, passes basic testing, and I would rather you/intel do this patch

For posterity, I did email Tony with handoff notes stating I'd rather not do the 
xdp/xsk/af_xdp/fpe side of things, but hadn't heard back so I kept going.

Re: [PATCH net] igc: track rx frame boundaries independent of skbs

[Maciej Fijalkowski]

On Mon, Oct 26, 2026 at 10:12:51PM +0000, Paul Moses wrote:
> 
> 
> 
> 
> On Monday, October 26th, 2026 at 2:46 PM, Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote:
> 
> > On Sun, Oct 25, 2026 at 06:52:06AM +0000, Paul Moses wrote:
> > > TSIP remains set on continuation descriptors, but the inline timestamp
> > > header exists only in the first buffer. Stripping it from every
> > > descriptor truncates split frames and can underflow short buffers.
> > >
> > > skb ownership cannot identify frame boundaries when XDP consumes the
> > > first buffer or AF_XDP uses zero-copy.
> > >
> > > Track rx_in_frame per ring and update it as descriptors are consumed.
> > > Strip timestamps only from first buffers, reject unsupported split
> > > frames before XDP and drain continuations. Account discarded
> > > descriptors against NAPI work without counting successful RX.
> > >
> > > Fixes: e1ed4f92a625 ("igc: Refactor Rx timestamp handling")
> > > Fixes: 26575105d6ed ("igc: Add initial XDP support")
> > > Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Paul Moses <p@1g4.org>
> > > ---
> > > - This supersedes "igc: only strip RX timestamp header from first buffer"
> > >   following feedback from Simon Horman, Maciej Fijalkowski and Sashiko.
> > >
> > > Changes:
> > > - track frame boundaries independently of skb ownership
> > > - guard timestamp stripping and drain unsupported split frames
> > > - cover the AF_XDP zero-copy RX path
> > > ---
> > >  drivers/net/ethernet/intel/igc/igc.h      |  1 +
> > >  drivers/net/ethernet/intel/igc/igc_main.c | 87 ++++++++++++++++-------
> > >  2 files changed, 64 insertions(+), 24 deletions(-)
> > 
> > Hi Paul,
> > 
> > I think this implementation went off track. We should make sure we *never*
> > enter data path for XDP/AF_XDP multi-buffer scenario until we add real
> > support to it within this driver. Now you sorta handle fragmented packets
> > in data path.
> > 
> > I tried to argue with bot reports that it hallucinated for
> > igc_clean_rx_irq() content (which was concise and to the point) and agreed
> > the AF_XDP should *not* be exposed to any fragmented traffic, which should
> > be addressed on HW RX ring configuration side, not within data path.
> > 
> > Also this should have been a v3 with a link to previous revisions so we
> > could keep track what has been changing between versions.
> > 
> > Here's something that would address ZC fragmentation issue. Could you give
> > it a spin on your side as I don't currently have igc in my setup handy.

[...]

> 
> Looks good to me, passes basic testing, and I would rather you/intel do this patch
> 
> For posterity, I did email Tony with handoff notes stating I'd rather not do the 
> xdp/xsk/af_xdp/fpe side of things, but hadn't heard back so I kept going. 

I think this was within the thread where AI hallucinated and I stepped in.
If you are not fed up yet you can take your v2 and the patch I added here
and send this as a change set. From my POV it would be better as you have
the HW to test this against and ironically my igc card went into
/dev/null's lab.

> 

Re: [PATCH net] igc: track rx frame boundaries independent of skbs

[Paul Moses]

On Tuesday, October 27th, 2026 at 3:24 AM, Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote:

> On Mon, Oct 26, 2026 at 10:12:51PM +0000, Paul Moses wrote:
> >
> >
> >
> >
> > On Monday, October 26th, 2026 at 2:46 PM, Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote:
> >
> > > On Sun, Oct 25, 2026 at 06:52:06AM +0000, Paul Moses wrote:
> > > > TSIP remains set on continuation descriptors, but the inline timestamp
> > > > header exists only in the first buffer. Stripping it from every
> > > > descriptor truncates split frames and can underflow short buffers.
> > > >
> > > > skb ownership cannot identify frame boundaries when XDP consumes the
> > > > first buffer or AF_XDP uses zero-copy.
> > > >
> > > > Track rx_in_frame per ring and update it as descriptors are consumed.
> > > > Strip timestamps only from first buffers, reject unsupported split
> > > > frames before XDP and drain continuations. Account discarded
> > > > descriptors against NAPI work without counting successful RX.
> > > >
> > > > Fixes: e1ed4f92a625 ("igc: Refactor Rx timestamp handling")
> > > > Fixes: 26575105d6ed ("igc: Add initial XDP support")
> > > > Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Paul Moses <p@1g4.org>
> > > > ---
> > > > - This supersedes "igc: only strip RX timestamp header from first buffer"
> > > >   following feedback from Simon Horman, Maciej Fijalkowski and Sashiko.
> > > >
> > > > Changes:
> > > > - track frame boundaries independently of skb ownership
> > > > - guard timestamp stripping and drain unsupported split frames
> > > > - cover the AF_XDP zero-copy RX path
> > > > ---
> > > >  drivers/net/ethernet/intel/igc/igc.h      |  1 +
> > > >  drivers/net/ethernet/intel/igc/igc_main.c | 87 ++++++++++++++++-------
> > > >  2 files changed, 64 insertions(+), 24 deletions(-)
> > >
> > > Hi Paul,
> > >
> > > I think this implementation went off track. We should make sure we *never*
> > > enter data path for XDP/AF_XDP multi-buffer scenario until we add real
> > > support to it within this driver. Now you sorta handle fragmented packets
> > > in data path.
> > >
> > > I tried to argue with bot reports that it hallucinated for
> > > igc_clean_rx_irq() content (which was concise and to the point) and agreed
> > > the AF_XDP should *not* be exposed to any fragmented traffic, which should
> > > be addressed on HW RX ring configuration side, not within data path.
> > >
> > > Also this should have been a v3 with a link to previous revisions so we
> > > could keep track what has been changing between versions.
> > >
> > > Here's something that would address ZC fragmentation issue. Could you give
> > > it a spin on your side as I don't currently have igc in my setup handy.
> 
> [...]
> 
> >
> > Looks good to me, passes basic testing, and I would rather you/intel do this patch
> >
> > For posterity, I did email Tony with handoff notes stating I'd rather not do the
> > xdp/xsk/af_xdp/fpe side of things, but hadn't heard back so I kept going.
> 
> I think this was within the thread where AI hallucinated and I stepped in.

No worries, it turned out more interesting than expected. 

> If you are not fed up yet you can take your v2 and the patch I added here
> and send this as a change set. From my POV it would be better as you have
> the HW to test this against and ironically my igc card went into
> /dev/null's lab.
> 
> >
> 
Not fed up but sleep deprived, yes, I'll do that.