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? [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? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
