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

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

On Thu, 2026-08-13 at 12:15 -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 Thu, Aug 13, 2026 at 4:40 AM Zhaoping Shu (舒召平)
> <Zhaoping.Shu@mediatek.com> wrote:
> > 
> > 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.
> 
> Oh right.
> 
> As long as the choice is only between fraglist GRO or not fraglist
> (rather than GRO or bypass GRO), it should not introduce any new
> reordering concerns.
> 
> But the decision cannot be made based on skb_is_gso(skb) of an
> arriving skb. Because when such a HW-GRO skb arrives a SW GRO context
> in fraglist mode may already have been opened, and it is too late to
> convert that to non-fraglist.
> 
> So essentially HW-GRO and fraglist are mutually exclusive.
> 
> > > 
> > > 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.
> 
> What is your plan for v4?
Based on kernel 7.2.0.rc7 code:
Add logic in tcp4/6_check_fraglist_gro() as follow:
if an arriving skb is the first packet in the GRO list.
	NAPI_GRO_CB(skb)->is_flist = !sk && !skb_is_gso(skb);

/*
* Otherwise, the arriving skb is not the first packet, which means
* that struct sk_buff *p exists.
*/
if (!skb_is_gso(skb) || !NAPI_GRO_CB(p)->is_flist) {
	/* Aggregate the skb using p's GRO method. */
	NAPI_GRO_CB(skb)->is_flist = NAPI_GRO_CB(p)->is_flist;
} else {
	NAPI_GRO_CB(skb)->is_flist = 0;
	/* Flush p and start a new GRO list using the non-fraglist
method. */
}

Another code change in
tcp_gro_receive() {
	...
	if (unlikely(NAPI_GRO_CB(p)->is_flist)) {
		...
		/* if aggregate method changed, flush current gro list
*/
		flush |= NAPI_GRO_CB(skb)->is_flist != NAPI_GRO_CB(p)-
>is_flist;
		if (flush || skb_gro_receive_list())
			...
	}
}

After this change:
- NETIF_F_GRO_HW and NETIF_F_GRO_FRAGLIST are no longer mutually
exclusive.
- In tethering scenarios, TCP fraglist GRO applies only to consecutive
non-GSO skbs(!skb_is_gso(skb)).
  others will adopt skb_gro_receive() path.

Please provide some suggestions on the changes above. Should I prepare
V4 patch based on these changes?