Re: [PATCH net v2] igc: only strip RX timestamp header from first buffer

[sashiko-bot@kernel.org]

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 <p@1g4.org>
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/20261013010854.4068874-1-p@1g4.org?part=1