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

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

[zhaoping.shu]

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://patchwork.kernel.org/patch/14706032
[1]: https://patchwork.kernel.org/patch/14702209
---
 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;
-
 	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)))
+		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..e983f55c4419 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -57,7 +57,8 @@ static __always_inline struct sk_buff *tcp6_gro_receive(struct list_head *head,
 	if (!th)
 		goto flush;
 
-	if (unlikely(skb->dev->features & NETIF_F_GRO_FRAGLIST))
+	if (unlikely((skb->dev->features & NETIF_F_GRO_FRAGLIST) &&
+		     !(skb->dev->features & NETIF_F_GRO_HW)))
 		tcp6_check_fraglist_gro(head, skb, th);
 
 	return tcp_gro_receive(head, skb, th);
-- 
2.17.0

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

[Willem de Bruijn]

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://patchwork.kernel.org/patch/14706032
> [1]: https://patchwork.kernel.org/patch/14702209
> ---
>  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?

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.

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

[Zhaoping Shu (舒召平)]

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.

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

[Willem de Bruijn]

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?

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

[Zhaoping Shu (舒召平)]

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?

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

[Willem de Bruijn]

> > > > 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?

Thanks. This sounds good to me.

The risk is that GRO might be less effective at coalescing, if HW-GRO
and non HW-GRO packets alternate regularly.

The alternative to make HW-GRO and fraglist GRO mutually exclusive
does not have that problem, but on the flipside cannot use the fraglist
optimization (esp for forwarding path).

So no free lunch. Either works.