On Tuesday, October 13th, 2026 at 8:10 PM, [email protected]
<[email protected]> wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
> to consider:
> - [High] igc: Frame Preemption express packets (mPackets) lose TSIP stripping
> when interleaved
> - [High] igc: incomplete TSIP underflow fix due to XDP fragment drop
> --
>
> commit 61d038c91b93b927335a69df60dbf6cc817c79e2
> Author: Paul Moses <[email protected]>
> igc: only strip RX timestamp header from first buffer
>
> This patch modifies the RX path to only subtract the 16-byte TSIP inline
> timestamp from the first buffer of an skb-assembled packet. This is intended
> to prevent an unsigned length underflow on continuation buffers shorter than
> 16 bytes.
>
> > 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]
> Does this condition corrupt interleaved express packets?
>
> Frame Preemption allows express packets (mPackets) to be interleaved in the
> RX ring while a preemptable packet is being assembled. When an mPacket
> arrives during this assembly, skb will be non-NULL.
>
> Because of the new !skb check, the mPacket's TSIP header won't be stripped,
> leaving pkt_offset at 0 and size unmodified.
>
> Shortly after this check in igc_clean_rx_irq(), the mPacket is handled:
>
> if (igc_fpe_is_pmac_enabled(adapter) &&
> igc_fpe_handle_mpacket(adapter, rx_desc, size, pktbuf + pkt_offset)) {
> /* Advance the ring next-to-clean */
> igc_is_non_eop(rx_ring, rx_desc);
> ...
>
> Will this pass the unadjusted size and unstripped TSIP header (pktbuf + 0)
> into igc_fpe_handle_mpacket(), treating the 16-byte hardware timestamp as
> packet payload?
It'll drop, not do that, but I'll fix in this series.
>
> [Severity: High]
> Does this fix fully prevent the continuation fragment underflow?
>
> The patch attempts to fix a queue stall caused by an integer underflow on
> continuation fragments, but relies on skb being non-NULL to detect them.
>
> If XDP is enabled and the administrator has enabled the rx-all feature
> (disabling MTU enforcement), an oversized packet can be split across multiple
> descriptors. If the XDP program drops the first descriptor (e.g., returning
> XDP_DROP), skb remains NULL.
>
> When the loop hits igc_is_non_eop() and continues to the next descriptor,
> skb will still be NULL. If that continuation fragment is shorter than
> 16 bytes and has the TSIP status bit set, won't this check evaluate to true
> and cause size to underflow exactly as it did before the patch?
>
No, xdp was still no bueno, but I've had some breakthroughs already today and
this is all mostly finished, I'll submit v3 series shortly.
Thanks,
Paul
