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