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

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

On Wed, 2026-08-12 at 22:29 -0400, Willem de Bruijn wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> zhaoping.shu@ wrote:
> > From: HW He <hw.he@mediatek.com>
> > 
> > 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.
> > 
> > When NETIF_F_GRO_HW is enabled, do not set NAPI_GRO_CB(skb)-
> > >is_flist.
> > Fall through to the regular skb_gro_receive() path instead of
> > skb_gro_receive_list().
> > 
> > Fixes: 8d95dc474f85 ("net: add code for TCP fraglist GRO")
> > Signed-off-by: HW He <hw.he@mediatek.com>
> > Signed-off-by: Zhaoping Shu <zhaoping.shu@mediatek.com>
> > ---
> > [2]: 
> > https://urldefense.com/v3/__https://patchwork.kernel.org/patch/14706032__;!!CTRNKA9wMg0ARbw!kigb147-G9EgNqdWMhaQ43udYOvN90SG6bVjeM-GxTsPo3E07_tbysuoEieszr5CbCqT94AYpt1b_M7VsgNaZcjtBS_AnYo$
> > [1]: 
> > https://urldefense.com/v3/__https://patchwork.kernel.org/patch/14702209__;!!CTRNKA9wMg0ARbw!kigb147-G9EgNqdWMhaQ43udYOvN90SG6bVjeM-GxTsPo3E07_tbysuoEieszr5CbCqT94AYpt1b_M7VsgNaZcjtu0bQPNI$
> > ---
> >  net/ipv4/tcp_offload.c   | 7 +++----
> >  net/ipv6/tcpv6_offload.c | 3 ++-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> > index 3b1fdcd3cb29..641c47fb1ea2 100644
> > --- a/net/ipv4/tcp_offload.c
> > +++ b/net/ipv4/tcp_offload.c
> > @@ -395,9 +395,6 @@ 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;
> > -
> 
> Interesting that ipv4 and ipv6 diverge here. Nice to try to make them
> more alike.
> 
> >       p = tcp_gro_lookup(head, th);
> >       if (p) {
> >               NAPI_GRO_CB(skb)->is_flist = NAPI_GRO_CB(p)-
> > >is_flist;
> > @@ -430,7 +427,9 @@ 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) &&
> > +                  !(skb->dev->features & NETIF_F_GRO_HW)))
> 
> Would it be better to test skb_is_gso(skb) and only skip fraglist GRO
> for HW-GRO skbs, rather than disabling it for all skbs?

Yes, for tethering packets, HW-GRO skbs are aggregated by
skb_gro_receive(), while others go through fraglist GRO. However, care
must be taken to avoid packets arriving out of order. I will work on
it and submit V4.

> 
> If treating the features as mutually exclusive, another option would
> be to replace these datapath checks with disabling one at
> configuration
> time, in netdev_fix_features.
> 
If V4 work well, there will be no need to check netdev->features.
Linux kernel GRO will be more robust and able to handle scenarios
where both NETIF_F_GRO_HW and NETIF_F_GRO_FRAGLIST are enabled.