Re: [PATCH net v4] net: gro: Fix nesting of TCP GSO SKBs in skb_gro_receive_list()

["Zhaoping Shu (舒召平)" <Zhaoping.Shu@mediatek.com>]

On Wed, 2026-08-19 at 19:00 -0400, Willem de Bruijn wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Wed, Aug 19, 2026 at 5:37 AM <zhaoping.shu@mediatek.com> wrote:
> > 
> > From: HW He <hw.he@mediatek.com>
> > 
> 
> Perhaps a brief one sentence summary of the problem, before
> a detailed repro. For instance:
> 
> Fraglist GRO plus hardware GRO can create an fraglist of
> HW-GRO packets. This cannot be segmented back into
> the original form.
> 
> Avoid constructing such a GSO packet, by flushing an already
> built fraglist GRO packet if a hardware GRO packet arrives.

Thanks for the input. It will be added in the next version.

> 
> > A device supports GRO_HW, and the device driver enables the
> > NETIF_F_GRO_FRAGLIST feature. During a tethering test,
> > skb_gro_receive_list() reaggregates the GSO packet. However,
> > skb_segment_list() cannot segment this packet back into
> > the original packets, which leads to IP fragmentation or packet
> > drop.
> > 
> > Scenario (Tethering/Forwarding):
> > 1.Driver submits a single TCP packet, P1. P1 is kept in the
> > gro_list as the first packet.
> > 
> > 2. The driver submits a TCP GSO skb, P2. P2 has already aggregated
> > multiple TCP packets by HW_GRO, and its non-linear data is stored
> > in
> > frags[].
> > 
> > 3. P1 and P2 match the GRO rules, and since there is no local
> > socket,
> > they are aggregated by skb_gro_receive_list(). The resulting skb,
> > P3, has a frag_list entry that still contains frags[]:
> > P3: [ Linear Data ] -> frag_list -> [ Linear Data ]
> >                                     [ frag[1] ]
> >                                     [ frag[2] ]
> >                                     ...
> > 4. Later, tcp4_gso_segment() or tcp6_gso_segment() calls
> > skb_segment_list() to segment P3. However, skb_segment_list() only
> > segments the entries in frag_list. It does not segment the frags[]
> > inside P2, so P3 is not restored to the original packets, which
> > leads
> > to IP fragmentation or packet drop in the following path.
> > 
> > Check skb_is_gso(skb) and current GRO method, make sure fraglist
> > GRO
> > applies to consecutive non-GSO skb, others adopt regular GRO path.
> > 
> > Fixes: 8d95dc474f85 ("net: add code for TCP fraglist GRO")
> > Signed-off-by: Zhaoping Shu <zhaoping.shu@mediatek.com>
> > Signed-off-by: HW He <hw.he@mediatek.com>
> > 
> > ---
> > [3]: 
> > https://urldefense.com/v3/__https://patchwork.kernel.org/patch/14747095__;!!CTRNKA9wMg0ARbw!mtzdcrdh_KSYnB26dUrJ4K3uNOJ9XGI3lU_VNfbojm-7Aicq4qyHBJAGhpa_4JB4iIXQXtG4o2yhNEm3wRNyuJU1Uyqga6o$
> > [2]: 
> > https://urldefense.com/v3/__https://patchwork.kernel.org/patch/14706032__;!!CTRNKA9wMg0ARbw!mtzdcrdh_KSYnB26dUrJ4K3uNOJ9XGI3lU_VNfbojm-7Aicq4qyHBJAGhpa_4JB4iIXQXtG4o2yhNEm3wRNyuJU1v-VM9gE$
> > [1]: 
> > https://urldefense.com/v3/__https://patchwork.kernel.org/patch/14702209__;!!CTRNKA9wMg0ARbw!mtzdcrdh_KSYnB26dUrJ4K3uNOJ9XGI3lU_VNfbojm-7Aicq4qyHBJAGhpa_4JB4iIXQXtG4o2yhNEm3wRNyuJU1wxRvTF8$
> > ---
> >  net/ipv4/tcp_offload.c   | 22 ++++++++++++++++------
> >  net/ipv6/tcpv6_offload.c | 15 +++++++++++++--
> >  2 files changed, 29 insertions(+), 8 deletions(-)
> > 
> > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> > index 3b1fdcd3cb29..e74d99ca9fac 100644
> > --- a/net/ipv4/tcp_offload.c
> > +++ b/net/ipv4/tcp_offload.c
> > @@ -332,6 +332,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;
> > +               flush |= NAPI_GRO_CB(p)->is_flist !=
> > NAPI_GRO_CB(skb)->is_flist;
> 
> Technically, is this one-line sufficient?

Yes, the complete context is
if (NAPI_GRO_CB(p)->is_flist) {
...
flush |= NAPI_GRO_CB(skb)->is_flist != NAPI_GRO_CB(p)->is_flist;
...
}
The flush scenario is p aggregate by fraglist gro, but skb is a gso
skb.
NAPI_GRO_CB(p)->is_flist == true;
NAPI_GRO_CB(skb)->is_flist == false;

> 
> >                 skb_set_network_header(skb,
> > skb_gro_receive_network_offset(skb));
> > 
> >                 if (flush || skb_gro_receive_list(p, skb))
> > @@ -395,12 +396,20 @@ static void tcp4_check_fraglist_gro(struct
> > list_head *head, struct sk_buff *skb,
> >         struct net *net;
> >         int iif, sdif;
> > 
> > -       if (likely(!(skb->dev->features & NETIF_F_GRO_FRAGLIST)))
> > -               return;
> > -
> >         p = tcp_gro_lookup(head, th);
> >         if (p) {
> > -               NAPI_GRO_CB(skb)->is_flist = NAPI_GRO_CB(p)-
> > >is_flist;
> > +               /* flist GRO applies to consecutive non-GSO skbs */
> > +               if (!skb_is_gso(skb) || !NAPI_GRO_CB(p)->is_flist)
> > {
> > +                       NAPI_GRO_CB(skb)->is_flist =
> > NAPI_GRO_CB(p)->is_flist;
> > +                       return;
> > +               }
> > +
> > +               /* Fall back to the regular GRO path */
> > +               if (NAPI_GRO_CB(p)->count == 1)
> > +                       NAPI_GRO_CB(p)->is_flist = 0;
> > +
> > +               NAPI_GRO_CB(skb)->is_flist = 0;
> > +
> 
> This reduces the chance to have to flush, by downgrading fraglist GRO
> packets if a hardware GRO packet is detected at the stage (count ==
> 1)
> where fraglist is not enabled in practice.
> 
> This is quite a bit of complexity. And if hardware GRO is enabled,
> what
> are the odds in practice that the device does not produce any
> hardware
> GRO packets, yet the software GRO stack is capable of assembling
> signficant GSO packets. I suspect low.
> 
> The solution is technically correct. But would simply disabling
> fraglist
> GRO when hardware GRO is enabled not be simpler? Or only keeping
> the flush and avoiding this micro-optimization.

In V2, for the tethering scenario, we directly flush GSO packets.
This is the simplest way to address the reported issue,
but those packets can no longer be aggregated by regular GRO.
This is the main difference from V4 and Jakub pointed out this issue.

V3 disables fraglist GRO when hardware GRO is enabled.
The additional suggestion for V3 is “replace these
datapath checks by disabling it at configuration time in
netdev_fix_features.”
However, if we disable NETIF_F_GRO_FRAGLIST in netdev_fix_features,
UDP packets will also can't aggregate by fraglist. This will affect
UDP performance.

V4 is more complex than V2, but it can maximize the number of
packets aggregated by GRO. It works similarly to how regular
GRO tries to further aggregate TCP GSO packets.

This is a trade-off between code complexity and
the GRO aggregation rate. You can choose either approach.
If you have another solution in mind, please let me know.

> 
> >                 return;
> >         }
> > 
> > @@ -410,7 +419,7 @@ static void tcp4_check_fraglist_gro(struct
> > list_head *head, struct sk_buff *skb,
> >         sk = __inet_lookup_established(net, iph->saddr, th->source,
> >                                        iph->daddr, ntohs(th->dest),
> >                                        iif, sdif);
> > -       NAPI_GRO_CB(skb)->is_flist = !sk;
> > +       NAPI_GRO_CB(skb)->is_flist = !sk && !skb_is_gso(skb);
> >         if (sk)
> >                 sock_gen_put(sk);
> >  }
> > @@ -430,7 +439,8 @@ struct sk_buff *tcp4_gro_receive(struct
> > list_head *head, struct sk_buff *skb)
> >         if (!th)
> >                 goto flush;
> > 
> > -       tcp4_check_fraglist_gro(head, skb, th);
> > +       if (unlikely(skb->dev->features & NETIF_F_GRO_FRAGLIST))
> > +               tcp4_check_fraglist_gro(head, skb, th);
> > 
> >         return tcp_gro_receive(head, skb, th);
> > 
> > diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
> > index f2a659cd6183..eec3778855eb 100644
> > --- a/net/ipv6/tcpv6_offload.c
> > +++ b/net/ipv6/tcpv6_offload.c
> > @@ -26,7 +26,18 @@ static void tcp6_check_fraglist_gro(struct
> > list_head *head, struct sk_buff *skb,
> > 
> >         p = tcp_gro_lookup(head, th);
> >         if (p) {
> > -               NAPI_GRO_CB(skb)->is_flist = NAPI_GRO_CB(p)-
> > >is_flist;
> > +               /* flist GRO applies to consecutive non-GSO skbs */
> > +               if (!skb_is_gso(skb) || !NAPI_GRO_CB(p)->is_flist)
> > {
> > +                       NAPI_GRO_CB(skb)->is_flist =
> > NAPI_GRO_CB(p)->is_flist;
> > +                       return;
> > +               }
> > +
> > +               /* Fall back to the regular GRO path */
> > +               if (NAPI_GRO_CB(p)->count == 1)
> > +                       NAPI_GRO_CB(p)->is_flist = 0;
> > +
> > +               NAPI_GRO_CB(skb)->is_flist = 0;
> > +
> >                 return;
> >         }
> > 
> > @@ -36,7 +47,7 @@ static void tcp6_check_fraglist_gro(struct
> > list_head *head, struct sk_buff *skb,
> >         sk = __inet6_lookup_established(net, &hdr->saddr, th-
> > >source,
> >                                         &hdr->daddr, ntohs(th-
> > >dest),
> >                                         iif, sdif);
> > -       NAPI_GRO_CB(skb)->is_flist = !sk;
> > +       NAPI_GRO_CB(skb)->is_flist = !sk && !skb_is_gso(skb);
> >         if (sk)
> >                 sock_gen_put(sk);
> >  #endif /* IS_ENABLED(CONFIG_IPV6) */
> > --
> > 2.17.0
> > 
> >