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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] igc: off-by-one in TSIP length check in igc_clean_rx_irq() and zc variant
--

commit 7b71c87cde8117fd4a451a2a079f34aaf64bd069
Author: Paul Moses <p@1g4.org>

igc: track rx frame boundaries independent of skbs

This commit tracks RX frame boundaries per ring via rx_in_frame so that
inline RX timestamps are stripped only from the first descriptor buffer.
It also rejects unsupported split frames before XDP, drains continuation
descriptors, and updates NAPI budget accounting in both standard and
AF_XDP zero-copy RX paths.

> 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

[ ... ]

> @@ -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;
>  		}

[Severity: High]
Should the length check in igc_clean_rx_irq() be size <= IGC_TS_HDR_LEN
instead of size < IGC_TS_HDR_LEN?

When a first descriptor has IGC_RXDADV_STAT_TSIP set and size ==
IGC_TS_HDR_LEN, the buffer contains only the 16-byte inline timestamp
header and no packet payload. With size < IGC_TS_HDR_LEN, the check passes
and size -= IGC_TS_HDR_LEN leaves size as 0.

If XDP is disabled and this is a non-EOP descriptor followed by a
continuation descriptor of at least 60 bytes (ETH_ZLEN),
igc_construct_skb() or igc_build_skb() creates an skb with zero linear
headlen, and igc_add_rx_frag() attaches the continuation buffer as a paged
fragment where skb->len == skb->data_len >= 60. Because skb->len >=
ETH_ZLEN, igc_cleanup_headers() -> eth_skb_pad() does nothing, and
igc_process_skb_fields() -> eth_type_trans() -> __skb_pull(skb, ETH_HLEN)
then triggers BUG() because skb->len drops below skb->data_len.

If EOP is set on the 16-byte descriptor, size == 0 runs XDP on an empty
buffer where XDP_TX calls igc_xdp_xmit_back() ->
igc_xdp_init_tx_descriptor() with a 0-byte length, and normal RX or
XDP_PASS allocates a 0-byte skb that eth_skb_pad() zero-pads to 60 bytes
and hands to napi_gro_receive().

[ ... ]

> @@ -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;

[Severity: High]
Similarly, in igc_clean_rx_irq_zc(), should the check be size <=
IGC_TS_HDR_LEN so a 16-byte TSIP descriptor does not result in size == 0
after subtracting IGC_TS_HDR_LEN, which would pass a 0-byte xdp_buff to
__igc_xdp_run_prog() and igc_dispatch_skb_zc()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025065154.2037149-1-p@1g4.org?part=1