On Fri, Oct 30, 2026 at 04:47:36PM +0000, Paul Moses wrote: > > > > > On Friday, October 30th, 2026 at 6:45 AM, Maciej Fijalkowski > <[email protected]> wrote: > > > 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 > > > > I did forget to tag yours for stable, so maybe the will satisfy it? > > I think ultimately, it sees all related bugs as one big problem in > an effort to catch adjacent. My understanding has been that unless > there's true interdependence of patches, ie. kernel will be left broken > unless taken together, a series in itself adequately satisfies this > requirement. > > Should I do a v4 to add the tag? Or can this be rectified through > intel's process?
CC: Jake Jake could you make sure we tag 2/2 as stable backport candidate? Let's not send v4 and start the whole process again, bots are rather happy so the ball is on tester's side now. Thanks a lot Paul! > > Paul
