Re: [PATCH net v2] xfrm: esp: reset outer TCP GSO metadata on espintcp input

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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] Problem: outer TCP GRO metadata can still break espintcp input
  when the outer GRO aggregate carries SKB_GSO_FRAGLIST.

--

xfrm: esp: reset outer TCP GSO metadata on espintcp input

On espintcp input, this patch clears stale outer TCP GSO metadata. It
calls skb_gso_reset() on the head skb and on each frag_list member at the
skip_cow label in esp_input() and esp6_input(). This is meant to stop the
decrypted inner packet from reaching skb_segment() with outer GSO state
and hitting BUG_ON(!list_skb->head_frag).

> diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
> index e76db5817e78..012b59f69292 100644
> --- a/net/ipv4/esp4.c
> +++ b/net/ipv4/esp4.c
> @@ -900,6 +900,14 @@ static int esp_input(struct xfrm_state *x, struct sk_buff *skb)
>  	nfrags = err;
>  
>  skip_cow:
> +	if (x->encap && x->encap->encap_type == TCP_ENCAP_ESPINTCP) {
> +		struct sk_buff *iter;
> +
> +		skb_gso_reset(skb);
> +		skb_walk_frags(skb, iter)
> +			skb_gso_reset(iter);
> +	}

[Severity: Medium]
Is this reset reached when the outer TCP GRO aggregate carries
SKB_GSO_FRAGLIST? It only runs after skb_cow_data() succeeds, and
skb_cow_data() can fail on that gso_type before we get here.

tcp4_gro_complete() sets this on fraglist GRO aggregates:

    skb_shinfo(skb)->gso_type |= SKB_GSO_FRAGLIST | SKB_GSO_TCPV4;

tcp4_check_fraglist_gro() chooses fraglist GRO when it finds no socket in
the ingress device's netns. That happens when rx-gro-list is enabled and
the espintcp socket is in another netns reached through veth or a bridge.
veth passes the aggregate on intact, and TCP queues it unsegmented.

When an ESP record spans such an aggregate, __strp_recv() makes it a
frag_list child of the message head and keeps the FRAGLIST gso_type. This
happens in one of two ways:

- skb_unclone() on an appended clone that has its own frag_list
- moving an old head that has a frag_list under a newly allocated head

esp_input() then takes the skb_cow_data() path, and for that child:

net/core/skbuff.c:skb_cow_data() {
    ...
		if (copyflag ||
		    skb_cloned(skb1) ||
		    ntail ||
		    skb_shinfo(skb1)->nr_frags ||
		    skb_has_frag_list(skb1)) {
			...
			if (ntail == 0)
				skb2 = skb_copy(skb1, GFP_ATOMIC);
			...
			if (unlikely(skb2 == NULL))
				return -ENOMEM;
    ...
}

net/core/skbuff.c:skb_copy() {
    ...
	if (WARN_ON_ONCE(skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST))
		return NULL;
    ...
}

esp_input() then takes the goto out before skip_cow, so neither reset
runs. The result is a WARN_ON_ONCE() splat, which panics with
panic_on_warn. Each valid ESP-in-TCP record like this is also dropped.
All of this happens before ESP authentication, on data that the TCP peer
controls.

esp6_input() has the same ordering:

	nfrags = skb_cow_data(skb, 0, &trailer);
	if (nfrags < 0) {
		ret = -EINVAL;
		goto out;
	}

The commit message says that at skip_cow:

    the head skb and all frag_list members are guaranteed to be uncloned
    with private skb_shinfo.

In this case skb_cow_data() fails before skip_cow is reached.

The skb_copy() WARN_ON_ONCE() itself is older than this patch. This is
still the same stale outer TCP GRO metadata problem the patch sets out to
fix, and a reset placed after skb_cow_data() can't cover it. Should the
outer GSO state be handled before or inside the COW step instead?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020030450.7170-1-yangguoshuai%40gmail.com