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()
to check for address/port changes.
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@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

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()
> to check for address/port changes.
> 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@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,

Isn't it better to set the network header in skb_gro_receive_list instead?

Other than that, LGTM.

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()
> to check for address/port changes.
> 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@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,

Were you able to reproduce this regression? If so, can you share how?

On 10.07.25 13:37, Richard Gobert wrote:
> 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()
>> to check for address/port changes.
>> 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@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,
> 
> Were you able to reproduce this regression? If so, can you share how?

This was reported by several OpenWrt users after upgrading from Linux 
6.6 to 6.12. OpenWrt enables fraglist GRO by default. From the reports, 
the issues appeared when bridging packets through a VLAN filtering 
bridge from DSA ports to a WLAN device.

- Felix

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@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
>

On 06.07.25 15:45, Willem de Bruijn wrote:
> 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).

I just spent some time reviewing the code in order to understand how to 
fix it properly, and I still don't fully understand what you wrote 
above, especially the part related to "Non segment list GSO".

The issue that I'm trying to fix is that the skb network header is wrong 
for all skbs stored in the frag_list of the first skb.
The main skb is fine, since the offsets are handled by the network 
stack. For all the extra fragment skbs, the offsets are unset because we 
bypassed the part of the stack that sets them.

Since non-segment-list GSO skbs don't carry extra frag_list skbs, I 
don't see how they can share the same issue.

If I misread what you wrote, please point me at the relevant code 
context that I'm missing.

Thanks,

- Felix

Felix Fietkau wrote:
> On 06.07.25 15:45, Willem de Bruijn wrote:
> > 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).
> 
> I just spent some time reviewing the code in order to understand how to 
> fix it properly, and I still don't fully understand what you wrote 
> above, especially the part related to "Non segment list GSO".
> 
> The issue that I'm trying to fix is that the skb network header is wrong 
> for all skbs stored in the frag_list of the first skb.
> The main skb is fine, since the offsets are handled by the network 
> stack. For all the extra fragment skbs, the offsets are unset because we 
> bypassed the part of the stack that sets them.
> 
> Since non-segment-list GSO skbs don't carry extra frag_list skbs, I 
> don't see how they can share the same issue.

Reviewed-by: Willem de Bruijn <willemb@google.com>


Good point.

This patch's approach of setting the field in the gro_receive call of
the inner L4 header (and not the UDP GRO code for the outer encap UDP
header) is definitely preferable over having to iterate over the list
of frag_list skbs again later.

The only issue is whether the setting is correct and safe in case of
tunnels.

On rereading, I think it is. Hence the Reviewed-by tag.



That case is complex enough that ideally we would have a testcase to
cover it. To be clear, I definitely don't mean to ask you to write
that. Just a note to self for the future and/or for anyone whose
workload actually depends on tunneled packets with fraglist GSO (which
is not me, actually).

The main fix in Richard's series was to have the GRO complete code no
longer depend on skb_network_header. Whether or not skb_network_header
is also still set is immaterial to the fix. We just assumed that
nothing besides that GRO complete code even referenced it.


Setting skb_network_offset unconditionally would not be correct for
the head_skb in the presence of tunneling. Because GSO handling of
tunneling requires this to be correctly set in the inner fields:

In __skb_udp_tunnel_segment:

        /* setup inner skb. */
        skb->encapsulation = 0;
        SKB_GSO_CB(skb)->encap_level = 0;
        __skb_pull(skb, tnl_hlen);
        skb_reset_mac_header(skb);
        skb_set_network_header(skb, skb_inner_network_offset(skb));
        skb_set_transport_header(skb, skb_inner_transport_offset(skb));

After this tunnel GSO handler, the rest of the GSO stack no longer
sees the tunnel header and treats the packet as unencapsulated.

The fraglist code is only relevant for the innermost L4 transport
header. Since nothing touches the frag_list skb headers between GRO
receive, and L4 GSO fraglist segment handlers __tcp?_gso_segment_list
and __udpv?_gso_segment_list_csum, it is fine to have network offset
set, without having to handle encapsulation explicitly.


> If I misread what you wrote, please point me at the relevant code 
> context that I'm missing.
> 
> Thanks,
> 
> - Felix

On 06.07.25 15:45, Willem de Bruijn wrote:
> 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.
Will change that in v2, thanks.
> 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.

Just to clarify, in inet/ipv6_gro_complete you want me to iterate over 
all fragment skbs, calculate the header offset based on the first skb, 
and set it?

- Felix

Felix Fietkau wrote:
> On 06.07.25 15:45, Willem de Bruijn wrote:
> > 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.
> Will change that in v2, thanks.
> > 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.
> 
> Just to clarify, in inet/ipv6_gro_complete you want me to iterate over 
> all fragment skbs, calculate the header offset based on the first skb, 
> and set it?

If that is the best way to fix this without causing regressions.

There may be a better solution. I just don't have a good suggestion
off the top of my head.

The blamed commit itself fixed an issue, where GRO code incorrectly
used the network header in GRO complete when it should be using the
inner network header.

Perhaps with moving that access to the CB, it is still safe to also
set the original network header. Perhaps Richard has an opinion.

If we want to be exact, these should still be updated to the inner
fields for encapsulated inner L4 protocols. Similar to what
tcp_gro_complete does for the transport header.

On 7/6/25 3:45 PM, Willem de Bruijn wrote:
> 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.

+1 on setting the headers at GSO time.

> 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.

I think this actually breaks tunneled use-case, when the aggregated
packet is forwarded to an output device before traversing the relevant
tunnel.

/P