[PATCH net 0/2] net: gro: fix frag0 and remcsum handling

[PATCH net 0/2] net: gro: fix frag0 and remcsum handling

[Eric Dumazet]

Two GRO fixes for paths using remote checksum offload (VXLAN, GUE)
and for NAPI_GRO_CB(skb)->frag0 after a pull.

Patch 1 skips skb_gro_remcsum_cleanup() once the skb was merged,
as skb->data was moved past the headers.

Patch 2 invalidates frag0 after a pull, which commit bd56a29c7a4e
("net: gro: change skb_gro_network_header()") stopped doing.


Eric Dumazet (2):
  net: gro: do not undo remote checksum offload on merged packets
  net: gro: invalidate frag0 after a pull

 include/net/gro.h       | 15 ++++++++++++++-
 net/core/gro.c          |  1 +
 net/ipv4/esp4_offload.c |  1 +
 net/ipv4/udp_offload.c  |  1 +
 net/ipv6/esp6_offload.c |  1 +
 5 files changed, 18 insertions(+), 1 deletion(-)

-- 
2.53.0

[PATCH net 1/2] net: gro: do not undo remote checksum offload on merged packets

[Eric Dumazet]

skb_gro_flush_final_remcsum() calls skb_gro_remcsum_cleanup() to undo
the checksum adjustment done by skb_gro_remcsum_process() when the
packet was not consumed by GRO.

When the inner handler merged the packet (NAPI_GRO_CB(skb)->same_flow
is set), skb->data might have been moved past the headers:

- skb_gro_receive() uses __skb_pull(skb, offset) before adding the skb
  to the frag_list of the held packet.

- skb_gro_receive_list() uses skb_pull(skb, skb_gro_offset(skb)).

grc->offset is relative to the original skb->data, so the 2-byte
read-modify-write done by skb_gro_remcsum_cleanup() can land in
the payload of a packet which now belongs to the GRO packet.
pskb_may_pull() can also be called on this frag_list member.

With fraglist GRO, this can also be a use-after-free:
pskb_may_pull(skb, skb_gro_offset(skb)) in skb_gro_receive_list()
or udp_gro_receive_segment() releases the first page fragment if it
only contains headers. NAPI_GRO_CB(skb)->frag0 and frag0_len are not
updated, so skb_gro_remcsum_cleanup() takes the fast path and writes
through frag0 into a page the skb no longer holds a reference on.
The non merged case, where frag0 is used after this pull, is handled
in the following patch.

Headers of a merged skb are not used anymore: the skb is either freed,
or its headers are rebuilt from the head skb in skb_segment() or
skb_segment_list(). Skip the cleanup in this case.

This affects VXLAN and GUE with remote checksum offload enabled.

Fixes: 26c4f7da3e41 ("net: Fix remcsum in GRO path to not change packet")
Tested-by: Cen Zhang <cenzhang@linux.microsoft.com>
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
 include/net/gro.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/net/gro.h b/include/net/gro.h
index 2300b6da05b2728ec40f42228f8fa9c195d8479c..b560e866683b03314b3e80a887ac4467d1a5acb9 100644
--- a/include/net/gro.h
+++ b/include/net/gro.h
@@ -351,7 +351,10 @@ static inline void skb_gro_remcsum_cleanup(struct sk_buff *skb,
 	void *ptr;
 	size_t plen = grc->offset + sizeof(u16);
 
-	if (!grc->delta)
+	/* Once merged, skb->data was moved past the headers, and
+	 * the headers of this skb will not be used anymore.
+	 */
+	if (!grc->delta || NAPI_GRO_CB(skb)->same_flow)
 		return;
 
 	ptr = skb_gro_header(skb, plen, grc->offset);
-- 
2.53.0

[PATCH net 2/2] net: gro: invalidate frag0 after a pull

[Eric Dumazet]

Since commit bd56a29c7a4e ("net: gro: change skb_gro_network_header()"),
skb_gro_header_slow() no longer invalidates NAPI_GRO_CB(skb)->frag0
after pskb_may_pull(). skb_gro_may_pull() keeps comparing against the
frag0_len value set in skb_gro_reset_offset(), assuming that after a
slow pull, all later accesses are beyond frag0_len.

This is not true:

- udp4_gro_receive() and udp6_gro_receive() read the network header
  with skb_gro_network_header() after udp_gro_udphdr() pulled the
  UDP header.

- vxlan_gro_receive() calls skb_gro_remcsum_cleanup() at a lower
  offset after the inner handlers pulled their headers.

- skb_gro_remcsum_process() can pull beyond the inner Ethernet header
  before eth_gro_receive() reads it.

- napi_frags_skb() calls __skb_pull(skb, ETH_HLEN) after a slow pull,
  so frag0 still points to the Ethernet header.

If the pull consumed the first page fragment, __skb_pull_tail() released
it. If the head had to grow, pskb_expand_head() freed the old one.
These reads, and the 2-byte write in skb_gro_remcsum_cleanup(), then
access memory the skb no longer holds a reference on.

esp4_gro_receive(), esp6_gro_receive(), skb_gro_receive_list() and
udp_gro_receive_segment() also pull directly and never invalidated
frag0.

Add skb_gro_frag0_invalidate(), which only clears frag0_len, and call
it after each of these pulls. This is a single store in the slow path,
and frag0 stays non NULL, so skb_gro_network_header() does not need
a NULL test.

Fixes: bd56a29c7a4e ("net: gro: change skb_gro_network_header()")
Reported-by: Taesoo Kim <tsgatesv@gmail.com>
Reported-by: Cen Zhang <cenzhang@linux.microsoft.com>
Tested-by: Cen Zhang <cenzhang@linux.microsoft.com>
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
 include/net/gro.h       | 10 ++++++++++
 net/core/gro.c          |  1 +
 net/ipv4/esp4_offload.c |  1 +
 net/ipv4/udp_offload.c  |  1 +
 net/ipv6/esp6_offload.c |  1 +
 5 files changed, 14 insertions(+)

diff --git a/include/net/gro.h b/include/net/gro.h
index b560e866683b03314b3e80a887ac4467d1a5acb9..78373e9caf68b6acefcd976ed7192ec5e44db2b9 100644
--- a/include/net/gro.h
+++ b/include/net/gro.h
@@ -160,12 +160,22 @@ static inline bool skb_gro_may_pull(const struct sk_buff *skb,
 	return likely(hlen <= NAPI_GRO_CB(skb)->frag0_len);
 }
 
+/* A pull can release the page fragment frag0 points to, or reallocate
+ * skb->head. Make sure later skb_gro_header() and skb_gro_network_header()
+ * calls no longer use frag0.
+ */
+static inline void skb_gro_frag0_invalidate(struct sk_buff *skb)
+{
+	NAPI_GRO_CB(skb)->frag0_len = 0;
+}
+
 static inline void *skb_gro_header_slow(struct sk_buff *skb, unsigned int hlen,
 					unsigned int offset)
 {
 	if (!pskb_may_pull(skb, hlen))
 		return NULL;
 
+	skb_gro_frag0_invalidate(skb);
 	return skb->data + offset;
 }
 
diff --git a/net/core/gro.c b/net/core/gro.c
index ff828a3dce3ca649e149a840bd6e3dfd9ddaf0c4..2fe1bbe49a082873a700d36f7a10570f0954526e 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -242,6 +242,7 @@ int skb_gro_receive_list(struct sk_buff *p, struct sk_buff *skb)
 		NAPI_GRO_CB(skb)->flush = 1;
 		return -ENOMEM;
 	}
+	skb_gro_frag0_invalidate(skb);
 
 	if (NAPI_GRO_CB(p)->last == p)
 		skb_shinfo(p)->frag_list = skb;
diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c
index abd77162f5e75ec01d5e66693f20a80a78af9e50..dea34411e77f6cf1302315e665df003e4d1130a6 100644
--- a/net/ipv4/esp4_offload.c
+++ b/net/ipv4/esp4_offload.c
@@ -39,6 +39,7 @@ static struct sk_buff *esp4_gro_receive(struct list_head *head,
 
 	if (!pskb_pull(skb, offset))
 		return NULL;
+	skb_gro_frag0_invalidate(skb);
 
 	if (xfrm_parse_spi(skb, IPPROTO_ESP, &spi, &seq) != 0)
 		goto out;
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index cf07c3c6611a22c29b57a72420f2f940609f5e9a..62c44c450d78c9c2b9ea38d953376026fd2aa4e7 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -747,6 +747,7 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
 					NAPI_GRO_CB(skb)->flush = 1;
 					return NULL;
 				}
+				skb_gro_frag0_invalidate(skb);
 				if ((skb->ip_summed != p->ip_summed) ||
 				    (skb->csum_level != p->csum_level)) {
 					NAPI_GRO_CB(skb)->flush = 1;
diff --git a/net/ipv6/esp6_offload.c b/net/ipv6/esp6_offload.c
index 22895521a57d09acfe091927f20188764f33f91b..674e66541d9f914bf788a148bceea51c1067f541 100644
--- a/net/ipv6/esp6_offload.c
+++ b/net/ipv6/esp6_offload.c
@@ -66,6 +66,7 @@ static struct sk_buff *esp6_gro_receive(struct list_head *head,
 
 	if (!pskb_pull(skb, offset))
 		return NULL;
+	skb_gro_frag0_invalidate(skb);
 
 	if (xfrm_parse_spi(skb, IPPROTO_ESP, &spi, &seq) != 0)
 		goto out;
-- 
2.53.0

Re: [PATCH net 0/2] net: gro: fix frag0 and remcsum handling

[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 1/2] net: gro: do not undo remote checksum offload on merged packets

[Paolo Abeni]

[adding Tom]

On 10/28/26 15:13, Eric Dumazet wrote:
> skb_gro_flush_final_remcsum() calls skb_gro_remcsum_cleanup() to undo
> the checksum adjustment done by skb_gro_remcsum_process() when the
> packet was not consumed by GRO.
I have a meta question _not_ intended to block this series. Is
remcsum offload still a thing? Can we get rid of it entirely?

AFAICS remcsum landed upstream before GSO_PARTIAL, and the latter
does a better job (specifically, it actually validates the inner
hdr csum, while AFAICS remcsum offload basically ignores inner
packet csum).

On the flip side remcsum offload has even lesser device requirements
than GSO_PARTIAL, so possibly some use-case still exists.

Am I missing something?

/P

Re: [PATCH net 1/2] net: gro: do not undo remote checksum offload on merged packets

[Willem de Bruijn]

Paolo Abeni wrote:
> [adding Tom]
> 
> On 10/28/26 15:13, Eric Dumazet wrote:
> > skb_gro_flush_final_remcsum() calls skb_gro_remcsum_cleanup() to undo
> > the checksum adjustment done by skb_gro_remcsum_process() when the
> > packet was not consumed by GRO.
> I have a meta question _not_ intended to block this series. Is
> remcsum offload still a thing? Can we get rid of it entirely?
> 
> AFAICS remcsum landed upstream before GSO_PARTIAL, and the latter
> does a better job (specifically, it actually validates the inner
> hdr csum, while AFAICS remcsum offload basically ignores inner
> packet csum).

And also LCO, which requires less infrastructure to offload all the
tunnel checksums.

Re: [PATCH net 1/2] net: gro: do not undo remote checksum offload on merged packets

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Both the commit message and the new code comment give a wrong
  reason for skipping the cleanup in the fraglist case.
- [Low] The new comment in skb_gro_remcsum_cleanup() (include/net/gro.h)
  says "Once merged, skb->data was moved past the headers".

Pre-existing issues:
- [Critical] This only applies to this commit taken on its own.

--

net: gro: do not undo remote checksum offload on merged packets

This patch makes skb_gro_remcsum_cleanup() return early when
NAPI_GRO_CB(skb)->same_flow is set. A merged skb may have had skb->data
pulled past its headers, so the 2-byte undo at grc->offset could land in
payload or in memory freed by a fraglist pull. The commit message says the
non-merged frag0 case is handled in the following patch.

> diff --git a/include/net/gro.h b/include/net/gro.h
> index 2300b6da05b27..b560e866683b0 100644
> --- a/include/net/gro.h
> +++ b/include/net/gro.h
> @@ -351,7 +351,10 @@ static inline void skb_gro_remcsum_cleanup(struct sk_buff *skb,
>  	void *ptr;
>  	size_t plen = grc->offset + sizeof(u16);
>  
> -	if (!grc->delta)
> +	/* Once merged, skb->data was moved past the headers, and

[Severity: Low]
Is this true for every merge path? In skb_gro_receive(), only the merge:
frag_list path calls __skb_pull(skb, offset). The frag-steal and head-steal
branches set same_flow without moving skb->data:

net/core/gro.c:skb_gro_receive() {
    ...
	if (headlen <= offset) {
		...
		skbinfo->nr_frags = 0;
		...
		NAPI_GRO_CB(skb)->free = NAPI_GRO_FREE;
		goto done;
	} else if (skb->head_frag) {
		...
		NAPI_GRO_CB(skb)->free = NAPI_GRO_FREE_STOLEN_HEAD;
		goto done;
	}
    ...
done:
    ...
	NAPI_GRO_CB(skb)->same_flow = 1;
}

On those paths the cleanup is skipped for a different reason: the skb's
frags or head now belong to p, and the skb is about to be freed.

The commit message says skb->data "might have been moved", but the comment
says it always was. Could the comment describe both cases?

> +	 * the headers of this skb will not be used anymore.
> +	 */

[Severity: Low]
Is this accurate for fraglist members? The commit message has a similar
claim:

  Headers of a merged skb are not used anymore: the skb is either freed,
  or its headers are rebuilt from the head skb in skb_segment() or
  skb_segment_list().

Before calling skb_gro_receive_list(), udp_gro_receive_segment() sets the
member's network header to the inner network offset:

	skb_set_network_header(skb, skb_gro_receive_network_offset(skb));

skb_segment_list() then copies only the link-layer and tunnel headers from
the head skb:

net/core/skbuff.c:skb_segment_list() {
    ...
		skb_push(nskb, -skb_network_offset(nskb) + offset);
    ...
		nskb->transport_header += len_diff;
		skb_copy_from_linear_data_offset(skb, -tnl_hlen,
						 nskb->data - tnl_hlen,
						 offset + tnl_hlen);
    ...
}

The member keeps its own inner IP and UDP headers. When addresses and
ports match, __udpv4_gso_segment_list_csum() returns early without touching
them. So the inner checksum written by skb_gro_remcsum_process() is what
gets sent for those segments.

The behavior still looks correct. The retained value is the full
remote-checksum result, the same value the non-GRO skb_remcsum_process()
path would write. Only the stated reason seems wrong.

Could the comment and commit message say that fraglist members keep their
adjusted inner headers, and that this adjusted checksum is the intended
value?

> +	if (!grc->delta || NAPI_GRO_CB(skb)->same_flow)
>  		return;
>  
>  	ptr = skb_gro_header(skb, plen, grc->offset);

[Severity: Critical]
This isn't a bug introduced by this patch, but the same_flow check does not
cover a non-merged skb whose frag0 has gone stale. In
udp_gro_receive_segment(), the is_flist branch pulls first and can then
still reject the packet:

net/ipv4/udp_offload.c:udp_gro_receive_segment() {
    ...
			if (NAPI_GRO_CB(skb)->is_flist) {
				if (!pskb_may_pull(skb, skb_gro_offset(skb))) {
					NAPI_GRO_CB(skb)->flush = 1;
					return NULL;
				}
				if ((skb->ip_summed != p->ip_summed) ||
				    (skb->csum_level != p->csum_level)) {
					NAPI_GRO_CB(skb)->flush = 1;
					return NULL;
				}
    ...
}

pskb_may_pull() can drop frags[0] if it holds only headers. If headlen is
non-zero, pskb_expand_head() can instead free the old head that frag0
points to. Neither NAPI_GRO_CB(skb)->frag0 nor frag0_len is updated.

If the checksum-state check then fails, or skb_gro_receive_list() returns
-E2BIG, same_flow stays 0. vxlan_gro_receive(), vxlan_gpe_gro_receive() and
gue_gro_receive() then call:

skb_gro_flush_final_remcsum()
  skb_gro_remcsum_cleanup()
    skb_gro_header()   <- frag0 fast path, stale frag0_len >= plen
    remcsum_unadjust() <- 2-byte read-modify-write

Can that write land in a page or head the skb no longer holds a reference
on?

The commit message says this case is handled in the following patch, "net:
gro: invalidate frag0 after a pull". In the final series state, that patch
adds skb_gro_frag0_invalidate() after this pskb_may_pull(), after the pull
in skb_gro_receive_list(), and inside skb_gro_header_slow(). So the issue is
gone by the end of the series.

The two patches carry different Fixes: tags. Should they be marked so that
stable backports take both together?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028141325.2103814-1-edumazet%40kernel.org

Re: [PATCH net 2/2] net: gro: invalidate frag0 after a pull

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] skb_gro_header_slow() (include/net/gro.h:172-180) calls
  skb_gro_frag0_invalidate() only after pskb_may_pull() succeeds.

--

net: gro: invalidate frag0 after a pull

The commit adds skb_gro_frag0_invalidate(), which clears
NAPI_GRO_CB(skb)->frag0_len. It is called after the pulls in
skb_gro_header_slow(), skb_gro_receive_list(), udp_gro_receive_segment(),
esp4_gro_receive() and esp6_gro_receive(). The goal is to stop later
skb_gro_header() and skb_gro_network_header() calls from using a frag0
that points into a released page fragment or a freed skb->head.

> diff --git a/include/net/gro.h b/include/net/gro.h
> index b560e866683b0..78373e9caf68b 100644
> --- a/include/net/gro.h
> +++ b/include/net/gro.h
> @@ -160,12 +160,22 @@ static inline bool skb_gro_may_pull(const struct sk_buff *skb,
>  	return likely(hlen <= NAPI_GRO_CB(skb)->frag0_len);
>  }
>  
> +/* A pull can release the page fragment frag0 points to, or reallocate
> + * skb->head. Make sure later skb_gro_header() and skb_gro_network_header()
> + * calls no longer use frag0.
> + */
> +static inline void skb_gro_frag0_invalidate(struct sk_buff *skb)
> +{
> +	NAPI_GRO_CB(skb)->frag0_len = 0;
> +}
> +
>  static inline void *skb_gro_header_slow(struct sk_buff *skb, unsigned int hlen,
>  					unsigned int offset)
>  {
>  	if (!pskb_may_pull(skb, hlen))
>  		return NULL;
>  
> +	skb_gro_frag0_invalidate(skb);

[Severity: High]
Should frag0 also be invalidated when pskb_may_pull() fails?

pskb_may_pull() can fail after it has already reallocated skb->head. In
__pskb_pull_tail(), pskb_expand_head() runs first, and the frag_list
handling after it can still return NULL:

net/core/skbuff.c:__pskb_pull_tail() {
    ...
	if (eat > 0 || skb_cloned(skb)) {
		if (pskb_expand_head(skb, 0, eat > 0 ? eat + 128 : 0,
				     GFP_ATOMIC))
			return NULL;
	}
    ...
				if (skb_shared(list)) {
					/* Sucks! We need to fork list. :-( */
					clone = skb_clone(list, GFP_ATOMIC);
					if (!clone)
						return NULL;
    ...
				if (!pskb_pull(list, eat)) {
					kfree_skb(clone);
					return NULL;
				}
    ...
}

The kerneldoc for that function says the skb does not change on failure.
That doesn't hold once pskb_expand_head() has freed the old head.

If that happens, the early return NULL here skips
skb_gro_frag0_invalidate(). NAPI_GRO_CB(skb)->frag0 then still points
into the freed head, and frag0_len keeps the old headlen.

The outer handlers keep running after the inner pull fails. With VXLAN
RCO and VXLAN_F_REMCSUM_NOPARTIAL, for example:

vxlan_gro_receive()
  skb_gro_remcsum_process()      /* frag0 fast path, sets grc->delta */
  eth_gro_receive() / inet_gro_receive() / udp_gro_receive_segment()
    skb_gro_header_slow()
      pskb_may_pull()            /* fails after pskb_expand_head() */
  skb_gro_flush_final_remcsum()
    skb_gro_remcsum_cleanup()

include/net/gro.h:skb_gro_remcsum_cleanup() {
    ...
	size_t plen = grc->offset + sizeof(u16);
    ...
	ptr = skb_gro_header(skb, plen, grc->offset);
	if (!ptr)
		return;

	remcsum_unadjust((__sum16 *)ptr, grc->delta);
}

plen falls inside the range that the remcsum fast path already checked,
so it is no larger than the stale frag0_len. Would skb_gro_header() then
return the stale frag0, so that remcsum_unadjust() writes 2 bytes into
the freed head? gue_gro_receive() in net/ipv4/fou_core.c also calls
this cleanup helper after its inner handlers.

The new calls in skb_gro_receive_list() and udp_gro_receive_segment()
have the same order, invalidating only when pskb_may_pull() succeeds.
udp_gro_receive_segment() can run as the inner UDP handler under VXLAN.

The helper before bd56a29c7a4e also invalidated only on success. Still,
the commit message describes exactly this case: a stale frag0 after
pskb_expand_head(), followed by the 2-byte write in
skb_gro_remcsum_cleanup().

Could these sites invalidate frag0 before calling pskb_may_pull(), or on
both outcomes?

>  	return skb->data + offset;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028141325.2103814-1-edumazet%40kernel.org