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
