Re: [RFC PATCH net-next] net: gro: coalesce padded small IPv4 TCP segments

[Richard Gobert <richardbgobert@gmail.com>]

Glenn Judd wrote:
> Software GRO fails to coalesce small IPv4/TCP segment that was
> padded up to the 60-byte minimum Ethernet frame.
> 
> The selftest tools/testing/selftests/drivers/net/hw/gro.py subtest
> sw_ipv4_data_lrg_1byte sends {100, 1} expecting to receive {101}.
> In current code, it receives {100, 1} (no coalescing) instead.
> 
> Cause: inet_gro_receive() computes its flush term from
> tot_len ^ skb_gro_len() before skb_gro_pull(), while skb_gro_len()
> still includes trailing Ethernet padding. A small IPv4/TCP segment
> padded up to the 60-byte minimum frame has tot_len != skb_gro_len(),
> so flush is set and the runt never coalesces.
> 
> Assisted-by: Claude:claude-opus-4-8
> Assisted-by: Codex:gpt-5.6
> Assisted-by: Meta:internal-AI-tooling
> Signed-off-by: Glenn Judd <gmj@meta.com>
> ---
> 
> Notes:
>     RFC notes
>     ---------
>     Per Jakub Kicinski, the open question is fast-path cost: this adds two
>     operations to the common IPv4 GRO path for every packet -- reading
>     iph->tot_len and the skb_gro_len() comparison.  Everything expensive
>     (linear check, trim, pointer refresh, csum recompute) is behind unlikely()
>     on the slow path.  Is that per-packet cost worth the coalescing win for
>     padded runts?
>     
>     Testing: netdevsim cannot reproduce this -- it never pads short frames to
>     ETH_ZLEN -- so sw_ipv4_data_lrg_1byte passes trivially there.  Reproduced
>     and fixed on a real NIC (cx7): baseline FAIL -> patched PASS.  Also
>     validated locally under KASAN + CONFIG_FAIL_SKB_REALLOC (no UAF).
> 
>  net/ipv4/af_inet.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 32d006c1a8ee..998ff77fd7b9 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1470,6 +1470,7 @@ struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb)
>  	const struct net_offload *ops;
>  	struct sk_buff *pp = NULL;
>  	const struct iphdr *iph;
> +	unsigned int tot_len;
>  	struct sk_buff *p;
>  	unsigned int hlen;
>  	unsigned int off;
> @@ -1498,6 +1499,25 @@ struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb)
>  		goto out;
>  
>  	NAPI_GRO_CB(skb)->proto = proto;
> +
> +	tot_len = ntohs(iph->tot_len);
> +	if (unlikely(skb_gro_len(skb) > tot_len)) {
> +		if (!skb_is_nonlinear(skb)) {
> +			if (tot_len < sizeof(*iph) ||
> +			    pskb_trim_rcsum(skb, off + tot_len))
> +				goto out;
> +
> +			NAPI_GRO_CB(skb)->frag0 = skb->data;
> +			NAPI_GRO_CB(skb)->frag0_len = skb->len;
> +			iph = skb_gro_header(skb, hlen, off);
> +			if (unlikely(!iph))
> +				goto out;
> +			if (skb->ip_summed == CHECKSUM_COMPLETE)
> +				NAPI_GRO_CB(skb)->csum =
> +					skb_checksum(skb, off, tot_len, 0);
> +		}
> +	}
> +
>  	flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (ntohl(*(__be32 *)&iph->id) & ~IP_DF));
>  
>  	list_for_each_entry(p, head, list) {
> 
> base-commit: 2fbade66245059c78daeaccfce13ecf499fffb51

To address Jakub's question on the fast-path cost: I benchmarked GRO
forwarding with two-minute long iperf sessions using 1, 2 and 4 TCP streams
(17 runs per configuration) and CPU frequency scaling disabled. I also
disabled RSS during the benchmarks because it caused a lot of noise - up to
20% variance in the deltas.

| streams | baseline (Gbit/s)  | patched (Gbit/s)  | delta  |
|---------|--------------------|-------------------|--------|
|    1    | 14.083 ± 1.01      | 14.065 ± 0.67     | −0.13% |
|    2    | 13.891 ± 0.75      | 13.926 ± 0.78     | +0.25% |
|    4    | 13.008 ± 1.26      | 13.029 ± 0.97     | +0.16% |

The two added fast-path operations (iph->tot_len read + the skb_gro_len()
comparison on every IPv4 GRO packet) produce no measurable throughput
change. The deltas are all well under the 95% confidence interval and
indistinguishable from noise.