Re: [PATCH] net: fix segmentation of forwarding fraglist GRO

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Paolo Abeni wrote:
> On 12/17/25 4:55 AM, Jibin Zhang wrote:
> > This patch enhances GSO segment checks by verifying the presence
> > of frag_list and protocol consistency, addressing low throughput
> > issues on IPv4 servers when used as hotspots
> > 
> > Specifically, it fixes a bug in GSO segmentation when forwarding
> > GRO packets with frag_list. The function skb_segment_list cannot
> > correctly process GRO skbs converted by XLAT, because XLAT only
> > converts the header of the head skb. As a result, skbs in the
> > frag_list may remain unconverted, leading to protocol
> > inconsistencies and reduced throughput.
> > 
> > To resolve this, the patch uses skb_segment to handle forwarded
> > packets converted by XLAT, ensuring that all fragments are
> > properly converted and segmented.
> > 
> > Signed-off-by: Jibin Zhang <jibin.zhang@mediatek.com>
> 
> This looks like a fix, it should target the 'net' tree and include a
> suitable Fixes tag.
> 
> > ---
> >  net/ipv4/tcp_offload.c | 3 ++-
> >  net/ipv4/udp_offload.c | 3 ++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> > index fdda18b1abda..162a384a15bb 100644
> > --- a/net/ipv4/tcp_offload.c
> > +++ b/net/ipv4/tcp_offload.c
> > @@ -104,7 +104,8 @@ static struct sk_buff *tcp4_gso_segment(struct sk_buff *skb,
> >  	if (!pskb_may_pull(skb, sizeof(struct tcphdr)))
> >  		return ERR_PTR(-EINVAL);
> >  
> > -	if (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST) {
> > +	if ((skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST) && skb_has_frag_list(skb) &&
> > +	    (skb->protocol == skb_shinfo(skb)->frag_list->protocol)) {
> >  		struct tcphdr *th = tcp_hdr(skb);

Using fraglist gso on a system that modifies packet headers is a known
bad idea. I guess this was not anticipated when the feature was added.
But we've seen plenty of examples with BPF already before.

This skb->protocol change is only one of a variety of ways that the
headers may end up mismatching.

It's not bad to bandaid it and fall back onto regular GSO.

But it seems like we'll continue to have to play whack-a-mole unless
we find a more fundamental solution. E.g., disabling fraglist GRO when
such a path is detected, or downgrade an skb to non-fraglist in paths
like this XLAT.


> >  
> >  		if (skb_pagelen(skb) - th->doff * 4 == skb_shinfo(skb)->gso_size)
> > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> > index 19d0b5b09ffa..704fb32d10d7 100644
> > --- a/net/ipv4/udp_offload.c
> > +++ b/net/ipv4/udp_offload.c
> > @@ -512,7 +512,8 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
> >  		return NULL;
> >  	}
> >  
> > -	if (skb_shinfo(gso_skb)->gso_type & SKB_GSO_FRAGLIST) {
> > +	if ((skb_shinfo(gso_skb)->gso_type & SKB_GSO_FRAGLIST) && skb_has_frag_list(gso_skb) &&
> > +	    (gso_skb->protocol == skb_shinfo(gso_skb)->frag_list->protocol)) {
> >  		 /* Detect modified geometry and pass those to skb_segment. */
> >  		if (skb_pagelen(gso_skb) - sizeof(*uh) == skb_shinfo(gso_skb)->gso_size)
> >  			return __udp_gso_segment_list(gso_skb, features, is_ipv6);
> 
> I guess checks should be needed for ipv6.
> 
> Also it looks like this skips the CSUM_PARTIAL preparation, and possibly
> break csum offload.
> 
> Additionally I don't like the ever increasing stack of hacks needed to
> let GSO_FRAGLIST operate in the most diverse setups, the simpler fix
> would be disabling such aggregation.
> 
> /P
>