On Monday, October 26th, 2026 at 1:52 AM, [email protected]
<[email protected]> wrote:
> 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 <[email protected]>
>
> 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/[email protected]?part=1
>
I'm assuming the hardware works as documented, if someone wants
to followup on hypothetical defensive scenarios, go for it.
Even if I did change these, where does it end?
