Re: [PATCH iwl-next 1/8] e1000e: add jumbo Rx CRC stripping

[Matt Vollrath]

On 9/3/26 06:27, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev/#/patchset/20260830232146.36948-2-tactii@xxxxxxxxx?part=1
> ---
> e1000e: add jumbo Rx CRC stripping
>      
> When the hardware is configured to not strip the FCS (CrcStripping=0),
> the jumbo Rx path would deliver frames with the FCS still attached.
> Follow the standard path's convention by stripping the FCS at EOP and
> leaving it out of the byte count.
>
> > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> > index 844f31ab37ad4..599600ad695cd 100644
> > --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> > @@ -1631,13 +1631,23 @@ static bool e1000_clean_jumbo_rx_irq(struct e1000_ring *rx_ring, int *work_done,
> >   			}
> >   		}
> >   
> > +		/* strip the Ethernet CRC; it may span fragments */
> > +		if (!(adapter->flags2 & FLAG2_CRC_STRIPPING) &&
> > +		    !(netdev->features & NETIF_F_RXFCS))
> > +			pskb_trim(skb, skb->len - 4);
> > +
> >   		/* Receive Checksum Offload */
> >   		e1000_rx_checksum(adapter, staterr, skb);
> >   
> >   		e1000_rx_hash(netdev, rx_desc->wb.lower.hi_dword.rss, skb);
> >   
> > -		/* probably a little skewed due to removing CRC */
> >   		total_rx_bytes += skb->len;
> > +		/* If configured to store CRC, keep the FCS bytes out of the
> > +		 * total_rx_bytes counter
> > +		 */
> > +		if (!(adapter->flags2 & FLAG2_CRC_STRIPPING) &&
> > +		    (netdev->features & NETIF_F_RXFCS))
> > +			total_rx_bytes -= 4;
>
> [Severity: Medium]
> Could these lockless evaluations of adapter->flags2 and netdev->features get
> out of sync if userspace changes features via ethtool while a packet is being
> processed?

Yes, this could be prevented by bringing down the adapter before changing bits
in e1000_set_features().

> If a concurrent change occurs between the first check and this second check,
> it appears it could lead to an inconsistent state where a packet's payload is
> incorrectly truncated by 4 bytes, or where the byte statistics are overcounted
> or undercounted by 4 bytes.
>
> >   		total_rx_packets++;
> >   
> >   		/* eth type trans needs skb->data to point to something */