netdev - Re: [PATCH net] net: fix segmentation after TCP/UDP fraglist GRO

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: <686a7e07728fc_3aa654294f9@willemb.c.googlers.com.notmuch>
Date: Sun, 06 Jul 2025 09:45:43 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Felix Fietkau <nbd@....name>, 
 netdev@...r.kernel.org, 
 Eric Dumazet <edumazet@...gle.com>, 
 Neal Cardwell <ncardwell@...gle.com>, 
 Kuniyuki Iwashima <kuniyu@...gle.com>, 
 "David S. Miller" <davem@...emloft.net>, 
 David Ahern <dsahern@...nel.org>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, 
 Simon Horman <horms@...nel.org>, 
 Willem de Bruijn <willemb@...gle.com>, 
 Richard Gobert <richardbgobert@...il.com>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: fix segmentation after TCP/UDP fraglist GRO

Felix Fietkau wrote:
> Since "net: gro: use cb instead of skb->network_header", the skb network
> header is no longer set in the GRO path.
> This breaks fraglist segmentation, which relies on ip_hdr()/tcp_hdr()

Only ip_hdr is in scope.

Reviewing TCP and UDP GSO, tcp_hdr/transport header is used also
outside segment list. Non segment list GSO also uses ip_hdr in case
pseudo checksum needs to be set.

The GSO code is called with skb->data at the relevant header, so L4
helpers are not strictly needed. The main issue is that data will be
at the L4 header, and some GSO code also needs to see the IP header
(e.g., for aforementioned pseudo checksum calculation).

> to check for address/port changes.

If in GSO, then the headers are probably more correctly set at the end
of GRO, in gro_complete.

The blamed commit was added to support tunneling. It's not obvious
that unconditionally setting network header again, instead of inner
network header, will break that.

Side-note: the number of times this feature has been broken indicates
we should aim for getting test coverage.

> Fix this regression by selectively setting the network header for merged
> segment skbs.

> Fixes: 186b1ea73ad8 ("net: gro: use cb instead of skb->network_header")
> Signed-off-by: Felix Fietkau <nbd@....name>
> ---
>  net/ipv4/tcp_offload.c | 1 +
>  net/ipv4/udp_offload.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> index d293087b426d..be5c2294610e 100644
> --- a/net/ipv4/tcp_offload.c
> +++ b/net/ipv4/tcp_offload.c
> @@ -359,6 +359,7 @@ struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb,
>  		flush |= skb->ip_summed != p->ip_summed;
>  		flush |= skb->csum_level != p->csum_level;
>  		flush |= NAPI_GRO_CB(p)->count >= 64;
> +		skb_set_network_header(skb, skb_gro_receive_network_offset(skb));
>  
>  		if (flush || skb_gro_receive_list(p, skb))
>  			mss = 1;
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 85b5aa82d7d7..e0a6bfa95118 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -767,6 +767,7 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
>  					NAPI_GRO_CB(skb)->flush = 1;
>  					return NULL;
>  				}
> +				skb_set_network_header(skb, skb_gro_receive_network_offset(skb));
>  				ret = skb_gro_receive_list(p, skb);
>  			} else {
>  				skb_gro_postpull_rcsum(skb, uh,
> -- 
> 2.49.0
>