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

[netdev-bot+sashiko@kernel.org]

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