[PATCH] net: gro: mark frag_list GRO packets as SKB_GSO_DODGY when a list element exceeds gso_size

[PATCH] net: gro: mark frag_list GRO packets as SKB_GSO_DODGY when a list element exceeds gso_size

[Shiming Cheng]

When RX LRO (or similar offload) is enabled, the TCP/IPv4 GRO path may
aggregate traffic using frag_list. The resulting skb is later segmented
via the frag_list segmentation path (skb_segment_list()).

However, some drivers can hand GRO/LRO-aggregated frames to the stack
where individual frag_list elements are already larger than skb_shinfo(p)
->gso_size (i.e., an element itself contains multiple MSS worth of
payload) and may be non-linear (nr_frags > 0). This shape is not
naturally produced by the software GRO aggregation logic for devices
without LRO, and can lead to unexpected behavior in the frag_list
segmentation path.

Detect this condition during frag_list aggregation and mark the
aggregated packet as SKB_GSO_DODGY when a list element’s length exceeds
gso_size. This forces a more conservative segmentation/linearization
behavior downstream and avoids relying on assumptions that do not hold
for LRO-produced aggregates.

No change for normal software GRO aggregation: the new check only
triggers when skb_shinfo(p)->gso_size is set and a frag_list element
length exceeds that size.

Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
Cc: <stable@vger.kernel.org>
Signed-off-by: Shiming Cheng <shiming.cheng@mediatek.com>
---
 net/core/gro.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/gro.c b/net/core/gro.c
index 29b4d02bf519..e70ecf19b0a7 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -259,6 +259,9 @@ int skb_gro_receive_list(struct sk_buff *p, struct sk_buff *skb)
 	skb_shinfo(p)->flags |= skb_shinfo(skb)->flags & SKBFL_SHARED_FRAG;
 
 	NAPI_GRO_CB(skb)->same_flow = 1;
+	/* frag_list element larger than gso_size (already coalesced before list-append) */
+	if (skb_shinfo(p)->gso_size && skb->len > skb_shinfo(p)->gso_size)
+		skb_shinfo(p)->gso_type |= SKB_GSO_DODGY;
 
 	return 0;
 }
-- 
2.45.2

Re: [PATCH] net: gro: mark frag_list GRO packets as SKB_GSO_DODGY when a list element exceeds gso_size

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] net: gro: mark frag_list GRO packets as SKB_GSO_DODGY when a list element exceeds gso_size

[Willem de Bruijn]

Shiming Cheng wrote:
> When RX LRO (or similar offload) is enabled, the TCP/IPv4 GRO path may
> aggregate traffic using frag_list. The resulting skb is later segmented
> via the frag_list segmentation path (skb_segment_list()).
> 
> However, some drivers can hand GRO/LRO-aggregated frames to the stack
> where individual frag_list elements are already larger than skb_shinfo(p)
> ->gso_size (i.e., an element itself contains multiple MSS worth of
> payload) and may be non-linear (nr_frags > 0). This shape is not
> naturally produced by the software GRO aggregation logic for devices
> without LRO, and can lead to unexpected behavior in the frag_list
> segmentation path.

Did you observe this with a specific driver?

We don't want to have to support every crazy driver scheme. The right
approach may be to fix the driver.

To understand the geometry: the driver passes a GSO skb with frag_list,
where frag_list members may be any size, not just gso_size? I.e., these
do not conform to SKB_GSO_FRAGLIST rules?

I don't recall immediately what the acceptable behavior for regular
GSO skbs with frag_list is. But for starters such a driver should not
advertiserr SKB_GSO_FRAGLIST.

> 
> Detect this condition during frag_list aggregation and mark the
> aggregated packet as SKB_GSO_DODGY when a list element’s length exceeds
> gso_size. This forces a more conservative segmentation/linearization
> behavior downstream and avoids relying on assumptions that do not hold
> for LRO-produced aggregates.
> 
> No change for normal software GRO aggregation: the new check only
> triggers when skb_shinfo(p)->gso_size is set and a frag_list element
> length exceeds that size.
> 
> Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Shiming Cheng <shiming.cheng@mediatek.com>
> ---
>  net/core/gro.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/gro.c b/net/core/gro.c
> index 29b4d02bf519..e70ecf19b0a7 100644
> --- a/net/core/gro.c
> +++ b/net/core/gro.c
> @@ -259,6 +259,9 @@ int skb_gro_receive_list(struct sk_buff *p, struct sk_buff *skb)
>  	skb_shinfo(p)->flags |= skb_shinfo(skb)->flags & SKBFL_SHARED_FRAG;
>  
>  	NAPI_GRO_CB(skb)->same_flow = 1;
> +	/* frag_list element larger than gso_size (already coalesced before list-append) */
> +	if (skb_shinfo(p)->gso_size && skb->len > skb_shinfo(p)->gso_size)
> +		skb_shinfo(p)->gso_type |= SKB_GSO_DODGY;
>  
>  	return 0;
>  }
> -- 
> 2.45.2
> 

Re: [PATCH] net: gro: mark frag_list GRO packets as SKB_GSO_DODGY when a list element exceeds gso_size

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch's premise does not hold on this tree, so the new
  check in skb_gro_receive_list() (net/core/gro.c:262-264) can never…
- [Medium] The Fixes: tag points at the wrong commit, and the message
  leaves out the commit the patch depends on.

--

net: gro: mark frag_list GRO packets as SKB_GSO_DODGY when a list element exceeds gso_size

This commit sets SKB_GSO_DODGY on the aggregated skb in
skb_gro_receive_list() when an appended frag_list element is longer than
skb_shinfo(p)->gso_size. The goal is to push LRO-produced aggregates
onto the conservative segmentation path instead of skb_segment_list().

> Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
> Cc: <stable@vger.kernel.org>

[Severity: Medium]
Is this the right Fixes: tag?

3a1296a38d0c added the generic fraglist chaining that UDP uses. The
condition tested here doesn't look reachable through UDP fraglist GRO.
GSO UDP skbs are flushed in dev_gro_receive():

net/core/gro.c:dev_gro_receive() {
    ...
		if (!skb_is_gso_tcp(skb) ||
		    (skb_shinfo(skb)->gso_type & SKB_GSO_DODGY))
			NAPI_GRO_CB(skb)->flush = 1;
    ...
}

Also, udp_gro_receive_segment() stops merging once an skb's ulen is larger
than the head's, so no element can be longer than gso_size.

An element longer than gso_size could only come from TCP fraglist GRO.
That was added by 8d95dc474f85 ("net: add code for TCP fraglist GRO").
The existing fix for this scenario, 66817a9794263c, also names
8d95dc474f85.

The DODGY bit set here only has an effect through the checks in
tcp4_gso_segment() and tcp6_gso_segment():

net/ipv4/tcp_offload.c:tcp4_gso_segment() {
    ...
		if ((skb_pagelen(skb) - th->doff * 4 == skb_shinfo(skb)->gso_size) &&
		    !(skb_shinfo(skb)->gso_type & SKB_GSO_DODGY))
			return __tcp4_gso_segment_list(skb, features);
    ...
}

426ca15c7f6c ("net: fix segmentation of forwarding fraglist GRO") added
the DODGY test there. 17bd3bd82f9f79 added the skb_pagelen() check next
to it.

With Cc: stable, the current tag would pull this patch into 5.10, 5.15,
6.1 and 6.6. Those trees do not have TCP fraglist GRO, so the change
does nothing there. Trees that have TCP fraglist GRO but not
426ca15c7f6c never read the new bit either.

Should the tag point at 8d95dc474f85 instead? Should the dependency on
426ca15c7f6c be mentioned? The commit those stable trees seem to need is
66817a9794263c.

> diff --git a/net/core/gro.c b/net/core/gro.c
> index 29b4d02bf5198..6c5b11b63d3b5 100644
> --- a/net/core/gro.c
> +++ b/net/core/gro.c
> @@ -259,6 +259,9 @@ int skb_gro_receive_list(struct sk_buff *p, struct sk_buff *skb)
>  	skb_shinfo(p)->flags |= skb_shinfo(skb)->flags & SKBFL_SHARED_FRAG;
>  
>  	NAPI_GRO_CB(skb)->same_flow = 1;
> +	/* frag_list element larger than gso_size (already coalesced before list-append) */
> +	if (skb_shinfo(p)->gso_size && skb->len > skb_shinfo(p)->gso_size)
> +		skb_shinfo(p)->gso_type |= SKB_GSO_DODGY;

[Severity: Medium]
Can this condition ever be true on the current tree?

Commit 66817a9794263c ("net: gro: Fix nesting of TCP GSO SKBs in
skb_gro_receive_list()") is already an ancestor of this patch. It covers
the same HW-GRO case by keeping GSO skbs out of fraglist GRO:

net/ipv4/tcp_offload.c:tcp4_check_fraglist_gro() {
    ...
		/* flist GRO applies to consecutive non-GSO skbs */
		if (!skb_is_gso(skb) || !NAPI_GRO_CB(p)->is_flist) {
    ...
}

tcp6_check_fraglist_gro() has the same guard. A GSO skb therefore ends
up with is_flist == 0. tcp_gro_receive() then flushes before it calls
skb_gro_receive_list():

	flush |= NAPI_GRO_CB(p)->is_flist != NAPI_GRO_CB(skb)->is_flist;

A GSO skb that starts a new flow also gets
is_flist = !sk && !skb_is_gso(skb), which is 0.

For non-GSO TCP skbs, tcp_gro_receive() also flushes on:

	flush |= (len - 1) >= mss;

Here mss is skb_shinfo(p)->gso_size and len is skb_gro_len(skb). After
the skb_pull(skb, skb_gro_offset(skb)) in skb_gro_receive_list(), len
equals skb->len. So skb->len > gso_size can't hold at this point.

For UDP, dev_gro_receive() flushes GSO skbs, as well as skbs that already
carry a frag_list:

	NAPI_GRO_CB(skb)->flush = skb_has_frag_list(skb);

udp_gro_receive_segment() also stops merging when
ulen > udp_get_len_short(uh2).

Is there a reproducer that still hits this after 66817a9794263c? If not,
this looks like dead code, and the stable tag would send it in place of
66817a9794263c. Should this patch be dropped in favor of the existing
fix?

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929100256.23192-1-shiming.cheng%40mediatek.com