linux-kernel - Re: [PATCH net-next v4 4/5] net: gro: remove unnecessary df checks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <willemdebruijn.kernel.868af9542505@gmail.com>
Date: Tue, 02 Sep 2025 14:27:32 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Richard Gobert <richardbgobert@...il.com>, 
 netdev@...r.kernel.org
Cc: davem@...emloft.net, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 horms@...nel.org, 
 corbet@....net, 
 saeedm@...dia.com, 
 tariqt@...dia.com, 
 mbloch@...dia.com, 
 leon@...nel.org, 
 ecree.xilinx@...il.com, 
 dsahern@...nel.org, 
 ncardwell@...gle.com, 
 kuniyu@...gle.com, 
 shuah@...nel.org, 
 sdf@...ichev.me, 
 aleksander.lobakin@...el.com, 
 florian.fainelli@...adcom.com, 
 willemdebruijn.kernel@...il.com, 
 alexander.duyck@...il.com, 
 linux-kernel@...r.kernel.org, 
 linux-net-drivers@....com, 
 Richard Gobert <richardbgobert@...il.com>
Subject: Re: [PATCH net-next v4 4/5] net: gro: remove unnecessary df checks

Richard Gobert wrote:
> Currently, packets with fixed IDs will be merged only if their
> don't-fragment bit is set. Merged packets are re-split into segments
> before being fragmented, so the result is the same as if the packets
> weren't merged to begin with.

This can perhaps be reworded a bit for clarity. Something like "With
the changes in the earlier patches in this series, the ID state (fixed
or incrementing) is now recorded for both inner and outer IPv4 headers,
so the restriction to only coalesce packets with fixed IDs can now be
lifted."
> 
> Remove unnecessary don't-fragment checks.
> 
> Signed-off-by: Richard Gobert <richardbgobert@...il.com>
> ---
>  include/net/gro.h                 | 5 ++---
>  net/ipv4/af_inet.c                | 3 ---
>  tools/testing/selftests/net/gro.c | 9 ++++-----
>  3 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/include/net/gro.h b/include/net/gro.h
> index 322c5517f508..691f267b3969 100644
> --- a/include/net/gro.h
> +++ b/include/net/gro.h
> @@ -448,17 +448,16 @@ static inline int inet_gro_flush(const struct iphdr *iph, const struct iphdr *ip
>  	const u32 id2 = ntohl(*(__be32 *)&iph2->id);
>  	const u16 ipid_offset = (id >> 16) - (id2 >> 16);
>  	const u16 count = NAPI_GRO_CB(p)->count;
> -	const u32 df = id & IP_DF;
>  
>  	/* All fields must match except length and checksum. */
> -	if ((iph->ttl ^ iph2->ttl) | (iph->tos ^ iph2->tos) | (df ^ (id2 & IP_DF)))
> +	if ((iph->ttl ^ iph2->ttl) | (iph->tos ^ iph2->tos) | ((id ^ id2) & IP_DF))
>  		return true;

This is just a cleanup?

If so, please make a brief note in the commit message. I end up
staring whether there is some deeper meaning relevant to the
functional change.

>  
>  	/* When we receive our second frame we can make a decision on if we
>  	 * continue this flow as an atomic flow with a fixed ID or if we use
>  	 * an incrementing ID.
>  	 */
> -	if (count == 1 && df && !ipid_offset)
> +	if (count == 1 && !ipid_offset)
>  		NAPI_GRO_CB(p)->ip_fixedid |= 1 << inner;
>  
>  	return ipid_offset ^ (count * !(NAPI_GRO_CB(p)->ip_fixedid & (1 << inner)));
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index fc7a6955fa0a..c0542d9187e2 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1393,10 +1393,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
>  
>  	segs = ERR_PTR(-EPROTONOSUPPORT);
>  
> -	/* fixed ID is invalid if DF bit is not set */
>  	fixedid = !!(skb_shinfo(skb)->gso_type & (SKB_GSO_TCP_FIXEDID << encap));
> -	if (fixedid && !(ip_hdr(skb)->frag_off & htons(IP_DF)))
> -		goto out;

I understand why the GRO constraint can now be relaxed. But why does
this also affect GSO?

Fixed ID is invalid on the wire if DF is not set. Is the idea behind
this change that GRO + GSO is just forwarding existing packets. Even
if the incoming packets were invalid on this point?

>  
>  	if (!skb->encapsulation || encap)
>  		udpfrag = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
> diff --git a/tools/testing/selftests/net/gro.c b/tools/testing/selftests/net/gro.c
> index d5824eadea10..3d4a82a2607c 100644
> --- a/tools/testing/selftests/net/gro.c
> +++ b/tools/testing/selftests/net/gro.c
> @@ -670,7 +670,7 @@ static void send_flush_id_case(int fd, struct sockaddr_ll *daddr, int tcase)
>  		iph2->id = htons(9);
>  		break;
>  
> -	case 3: /* DF=0, Fixed - should not coalesce */
> +	case 3: /* DF=0, Fixed - should coalesce */
>  		iph1->frag_off &= ~htons(IP_DF);
>  		iph1->id = htons(8);
>  
> @@ -1188,10 +1188,9 @@ static void gro_receiver(void)
>  			correct_payload[0] = PAYLOAD_LEN * 2;
>  			check_recv_pkts(rxfd, correct_payload, 1);
>  
> -			printf("DF=0, Fixed - should not coalesce: ");
> -			correct_payload[0] = PAYLOAD_LEN;
> -			correct_payload[1] = PAYLOAD_LEN;
> -			check_recv_pkts(rxfd, correct_payload, 2);
> +			printf("DF=0, Fixed - should coalesce: ");
> +			correct_payload[0] = PAYLOAD_LEN * 2;
> +			check_recv_pkts(rxfd, correct_payload, 1);
>  
>  			printf("DF=1, 2 Incrementing and one fixed - should coalesce only first 2 packets: ");
>  			correct_payload[0] = PAYLOAD_LEN * 2;
> -- 
> 2.36.1
>