Re: [PATCH net v2] net: gro: avoid nesting TCP GSO skbs in skb_gro_receive_list()

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

Zhaoping Shu (舒召平) wrote:
> On Wed, 2026-07-29 at 14:57 -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:
> > > On Thu, 2026-07-23 at 16:02 +0200, Willem de Bruijn wrote:
> > > > On Thu, Jul 23, 2026 at 3:39 PM Jakub Kicinski <kuba@kernel.org>
> > > > wrote:
> > > > > 
> > > > > On Thu, 23 Jul 2026 17:16:01 +0800 zhaoping.shu@mediatek.com
> > > > > wrote:
> > > > > > From: HW He <hw.he@mediatek.com>
> > > > > > 
> > > > > > On devices that support both NETIF_F_GRO_HW and
> > > > > > NETIF_F_GRO_FRAGLIST,
> > > > > 
> > > > > "devices that support FRAGLIST"? Isn't it a software feature?
> > > 
> > > Sorry for not making it clear. The test environment is:
> > > device supports GRO_HW, and enable NETIF_F_GRO_FRAGLIST in
> > > driver.
> > > 
> > > > > 
> > > > > > the hardware or driver may deliver packets that have already
> > > > > > been
> > > > > 
> > > > > If you have a driver in mind please name it.
> > > > > 
> > > > > > aggregated into a TCP GSO skb with frags[]. GRO may then
> > > > > > aggregate the skb again in skb_gro_receive_list().
> > > > > > 
> > > > > > This can create a nested GSO skb, which is not handled
> > > > > > correctly
> > > > > > by the
> > > > > > later GSO segmentation paths. When the skb is segmented by
> > > > > > skb_segment_list(), it not be fully restored to the original
> > > > > > packets.
> > > > > > 
> > > > > > Avoid this by setting NAPI_GRO_CB(skb)->flush for GSO skbs
> > > > > > before
> > > > > > aggregation.
> > > > > 
> > > > > I don't think we can do this. For GRO_HW devices re-aggregating
> > > > > in SW is quite helpful, HW often runs out of contexts or times
> > > > > out too soon, generating skbs with 16kB..32kB of data, the SW
> > > > > can help bring it up to full TSO.
> > > 
> > > I'll try to explain this issue below.
> > > 
> > > > 
> > > > Also, after e751256486d0 ("net: gro: fix double aggregation of
> > > > flush-marked skbs"), it's not clear an another bug remains.
> > > > 
> > > > If it is: as said "nested GRO" of hw + sw GRO is intentional,
> > > > e.g.,
> > > > for BIG-TCP.
> > > > 
> > > > But it may not be anticipated for skb_gro_receive_list. One
> > > > option
> > > > would be to skip the fraglist GSO optimization for such packets.
> > > > 
> > > > First I'd like to understand better what exact bug remains.
> > > 
> > > I agree that re-aggregation is useful for improving GRO efficiency.
> > > 
> > > However, as a general rule, a GSO skb must be segment back to the
> > > exact original packets stream. In tethering test,
> > > skb_segment_list()
> > > cannot correctly segment a nested GSO skb produced by this path.
> > 
> > So the specific issue is a driver that builds a regular (HW) GSO
> > packet followed by software GSO that uses fraglist?
> > 
> > Then I see three paths to fixing this
> > 
> > 1. decline to further apply SW GRO if skb is GSO and in fraglist mode
> > 2. if in fraglist mode, further apply SW GRO, but do not use fraglist
> > 3. in skb_segment_list detect this case and fall back onto
> >    skb_segment
> > 
> > We already apply option 3 to various cases where skb_segment_list
> > cannot handle complex use-cases of fraglist.
> > 
> > This patch chooses option 1, which is fine. Alternatively it could
> > fall through to the regular skb_gro_receive path below.
> > 
> 
> Thanks for the feedback.
>  
> Patch (option 1) is a minimal fix for the reported issue.

And that would be sufficient.
  
> For the other options, my initial thought is to handle this in
> tcp4_check_fraglist_gro()/tcp6_check_fraglist_gro(): if the netdev
> has both NETIF_F_GRO_HW and NETIF_F_GRO_FRAGLIST enabled, do not set
> NAPI_GRO_CB(skb)->is_flist, so that this tethering/forwarding case
> can keep using SW GRO and fall through to the regular skb_gro_receive()
> path instead of skb_gro_receive_list().

Yes, that sounds good. And then the above option 1 is not needed.

No HW-GRO implementation generates fraglist GRO packets, so the two
are fundamentally at odds anyway.

Ignoring the fraglist hint for HW-GRO skbs sounds good to me, thanks.
  
> If that direction makes sense, I can work on it,
> or send a follow-up patch to fix the reported issue with option 1.
> 
> > > This issue can reproduce in the following scenario:
> > > 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.
> > > 
> > > The patch only prevents that nested case before
> > > skb_gro_receive_list()
> > > aggregation. It does not affect packets that are re-aggregated by
> > > skb_gro_receive().
> > 
> > Thanks for the detailed explanation.
> > 
> > 
>