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@xxxxxxxxxx <netdev-bot+sashiko@xxxxxxxxxx> 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