On Thu, Oct 29, 2026 at 11:37:20PM +0000, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 1 potential > issue(s) to consider. > > Critical: 0 · High: 1 · Medium: 0 · 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). > > - [High] At this commit by itself, the new first-buffer test `!skb && > igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)` in igc_clean_rx_irq()… > > -- > > igc: only strip RX timestamp header from first buffer > > igc_clean_rx_irq() used to subtract IGC_TS_HDR_LEN from every descriptor > with TSIP set, which truncated multi-buffer packets. With this patch the > timestamp header is only stripped when skb is NULL, which the commit treats > as marking the first buffer of a new packet. > > Link: > https://lore.kernel.org/all/[email protected]/ > > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c > > b/drivers/net/ethernet/intel/igc/igc_main.c > > index 1fb5f3cbe93c5..41b293dc10c3a 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_main.c > > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > > @@ -2639,7 +2639,8 @@ static int igc_clean_rx_irq(struct igc_q_vector > > *q_vector, const int budget) > > > > pktbuf = page_address(rx_buffer->page) + rx_buffer->page_offset; > > > > - if (igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)) { > > + if (!skb && > > + igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)) { > > ctx.rx_ts = pktbuf; > > pkt_offset = IGC_TS_HDR_LEN; > > size -= IGC_TS_HDR_LEN; > > [Severity: High] > > Is skb == NULL a reliable first-buffer marker when an XDP program is > attached? The commit message says: > > Only consume the timestamp header when skb is NULL, which identifies the > first buffer of a new packet. > > Suppose igc_xdp_run_prog() returns IGC_XDP_CONSUMED, IGC_XDP_TX or > IGC_XDP_REDIRECT for the first, non-EOP buffer of a multi-descriptor frame. > Then no skb is built, and igc_clean_rx_irq() moves on to the continuation > descriptor with skb still NULL: > > /* fetch next buffer in frame if non-eop */ > if (igc_is_non_eop(rx_ring, rx_desc)) > continue; > > The continuation descriptor still has TSIP set, so this check strips > IGC_TS_HDR_LEN a second time. If that descriptor holds fewer than 16 bytes, > can the unsigned size wrap here? > > The continuation buffer then goes through the new-frame path: > > if (!skb) { > xdp_init_buff(&ctx.xdp, truesize, &rx_ring->xdp_rxq); > xdp_prepare_buff(&ctx.xdp, pktbuf - > igc_rx_offset(rx_ring), > igc_rx_offset(rx_ring) + pkt_offset, > size, true); > > In that case the XDP program would see a data_end about 4 GiB past data. > On XDP_PASS, the same size would reach igc_build_skb() or > igc_construct_skb(), and the __skb_put() there does not check it. > > At this commit, XDP with multi-descriptor frames looks possible on kernels > with PAGE_SIZE >= 8192. igc_set_rx_mode() only clamps RLPML under the > small-page #if: > > #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); > > On large-page kernels, RLPML therefore stays at MAX_JUMBO_FRAME_SIZE while > igc_rx_bufsz() returns IGC_RXBUFFER_2048. igc_xdp_set_prog() only checks > dev->mtu > ETH_DATA_LEN, not the hardware receive length limit. > > This needs RX hardware timestamping enabled, an XDP program that returns a > non-PASS verdict for a first buffer, and a peer that sends frames larger > than about 2032 bytes. > > The next patch in this series, "igc: fix AF_XDP receive buffer size for 2K > UMEM chunks", adds igc_set_rlpml(). It caps RLPML at 1526 bytes whenever > XDP is enabled, whatever PAGE_SIZE is. It is called from igc_setup_rctl() > and igc_xdp_set_prog(). With that change, XDP frames always fit in a single > descriptor, and skb == NULL becomes a correct first-buffer marker.
Wow - finally. > > This patch is tagged Cc: stable and could be backported on its own. Should > it depend on that follow-up change, or should the two patches be swapped in > the series? How would we make sure both commits from this set get actually backported together? > > -- > Sashiko AI review · > https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028083344.3047350-1-p%401g4.org
