From: Stefan Sperling <stsp@stsp.name>
Subject: Re: em(4): fix I210/I350 handling of crc stripping
To: Jonathan Matthew <jonathan@d14n.org>
Cc: tech@openbsd.org
Date: Mon, 7 Jul 2025 12:52:22 +0200

On Mon, Jul 07, 2025 at 08:03:33PM +1000, Jonathan Matthew wrote:
> dtucker@ reported that I210 em(4) interfaces behaved weirdly at specific
> larger-than-default-MTU packet sizes.
> 
> The handling of packets with very short final segments for I210/I350 looks
> fishy to me. On these chips the CRC is always stripped (see if_em.c r1.378 line 2850),
> but the code in em_rxeof() dealing with stripping the CRC will still remove the
> final 4 bytes of the packet if they happen to be split across segments.
> 
> The diff below, which reorders the tests in the if-else block so we never
> truncate packets in the name of CRC stripping on I210/I350, reportedly fixes this.
> Darren's test for reproducing the problem was just using ping:
> 
>   $ for i in `seq 64 9000`; do ping -c1 -w1 -s$i 192.168.32.248 >/dev/null || printf "$i "; done; echo
>   2007 2008 2009 4055 4056 4057 6103 6104 6105 8151 8152 8153 
> 
> If you use large packets with em(4) interfaces, or would like to but haven't
> been able to, you might want to test this yourself.
> 
> ok?
> 

Diff makes sense, ok stsp@

I cannot test these particular em devices myself.

> Index: if_em.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/if_em.c,v
> diff -u -p -r1.378 if_em.c
> --- if_em.c	31 Aug 2024 16:23:09 -0000	1.378
> +++ if_em.c	7 Jul 2025 03:31:22 -0000
> @@ -3116,13 +3116,14 @@ em_rxeof(struct em_queue *que)
>  
>  		if (status & E1000_RXD_STAT_EOP) {
>  			eop = 1;
> -			if (desc_len < ETHER_CRC_LEN) {
> +			if (sc->hw.mac_type == em_i210 ||
> +			    sc->hw.mac_type == em_i350) {
> +				/* crc has already been stripped */
> +				len = desc_len;
> +			} else if (desc_len < ETHER_CRC_LEN) {
>  				len = 0;
>  				prev_len_adj = ETHER_CRC_LEN - desc_len;
> -			} else if (sc->hw.mac_type == em_i210 ||
> -			    sc->hw.mac_type == em_i350)
> -				len = desc_len;
> -			else
> +			} else
>  				len = desc_len - ETHER_CRC_LEN;
>  		} else {
>  			eop = 0;
> 
>