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

[PATCH net v2 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), grc->offset can no longer be trusted:

- skb_gro_receive() either gives the frags or the head of the skb
  to the held packet, or 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.

Skipping the cleanup for a merged skb is correct:

- With skb_gro_receive(), the skb is freed, or its headers are
  rebuilt from the head skb in skb_segment().

- skb_segment_list() only copies the link-layer and tunnel headers
  from the head skb, so fraglist members keep their own inner headers.
  grc->delta is only set with VXLAN_F_REMCSUM_NOPARTIAL, where
  skb_remcsum_process() writes the same adjusted checksum in the non
  GRO path. Keeping it is the intended result.

This affects VXLAN and GUE with remote checksum offload enabled.

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

diff --git a/include/net/gro.h b/include/net/gro.h
index 2300b6da05b2..00b49b6b9e74 100644
--- a/include/net/gro.h
+++ b/include/net/gro.h
@@ -351,7 +351,13 @@ 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, grc->offset can not be trusted: the frags or head
+	 * of this skb now belong to the GRO packet, or skb->data was pulled
+	 * past the headers before adding it to a frag_list.
+	 * Fraglist members keep their adjusted inner checksum, which is the
+	 * value skb_remcsum_process() would have written.
+	 */
+	if (!grc->delta || NAPI_GRO_CB(skb)->same_flow)
 		return;
 
 	ptr = skb_gro_header(skb, plen, grc->offset);
-- 
2.53.0

Re: [PATCH net v2 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 1 potential
issue(s) to consider.

Critical: 0 (1) · 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] The commit message says that skipping
  skb_gro_remcsum_cleanup() for merged skbs is correct, and that…

Pre-existing issues:
- [Critical] At this commit, skb_gro_remcsum_cleanup() can still write
  through a stale NAPI_GRO_CB(skb)->frag0 when a fraglist handler has…

--

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

With this patch, skb_gro_remcsum_cleanup() returns early when
NAPI_GRO_CB(skb)->same_flow is set. A merged skb therefore no longer has
its remote checksum offload adjustment undone. Once an skb has been
merged, grc->offset does not point at the original header any more. The
2-byte write could then land in payload or, with fraglist GRO, in a page
the skb no longer references.

> diff --git a/include/net/gro.h b/include/net/gro.h
> index 2300b6da05b27..00b49b6b9e74f 100644
> --- a/include/net/gro.h
> +++ b/include/net/gro.h
> @@ -351,7 +351,13 @@ 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, grc->offset can not be trusted: the frags or head
> +	 * of this skb now belong to the GRO packet, or skb->data was pulled
> +	 * past the headers before adding it to a frag_list.
> +	 * Fraglist members keep their adjusted inner checksum, which is the
> +	 * value skb_remcsum_process() would have written.
> +	 */
> +	if (!grc->delta || NAPI_GRO_CB(skb)->same_flow)
>  		return;

[Severity: Medium]
The new comment says fraglist members keep "the value
skb_remcsum_process() would have written". The commit message says
the same thing:

  grc->delta is only set with VXLAN_F_REMCSUM_NOPARTIAL, where
  skb_remcsum_process() writes the same adjusted checksum in the non
  GRO path. Keeping it is the intended result.

Is that still true when skb_segment_list() segments the fraglist train
before any remote checksum processing runs?

Two paths seem to do this.

The first is a transit VXLAN or GUE train that was aggregated because a
wildcard-bound local tunnel socket matched in udp4_gro_lookup_skb(), and
is then forwarded. 26c4f7da3e41, the commit named in Fixes:, was written
for this case: GRO must not change a packet that may be forwarded.

The second is GUE local delivery. fou sockets never call
udp_allow_gso(), so udp_unexpected_gso() returns true for fraglist
packets:

include/linux/udp.h:udp_unexpected_gso() {
    ...
	if (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST &&
	    !udp_test_bit(ACCEPT_FRAGLIST, sk))
		return true;
    ...
}

As a result, udp_queue_rcv_skb() segments the train before
gue_udp_recv() runs.

In both cases, skb_segment_list() copies the head's outer and tunnel
headers onto every member, including the RCO flag and start/offset.
__copy_skb_header() also copies the head's remcsum_offload bit, which
skb_gro_flush_final_remcsum() has cleared. That means the check below no
longer stops the adjustment from running a second time:

net/ipv4/fou_core.c:gue_remcsum() {
    ...
	if (skb->remcsum_offload)
		return guehdr;
    ...
}

vxlan_remcsum() on a remote VTEP has the same check. If remcsum_adjust()
runs on a field that already holds the final checksum, won't it produce
the pseudo-header sum instead? Every member except the head would then
carry a bad inner checksum. This requires rx-gro-list enabled on the
ingress device and a NOPARTIAL remote checksum offload flow.

This patch did not introduce the wrong member checksum. The old cleanup
never restored the members either; it wrote into the payload or a stale
frag0 page instead.

Could the comment and the commit message qualify the claim that keeping
the adjusted value is correct? The member headers still appear to be in
the linear headroom after skb_pull(), so they could be restored.

Also, GUE sets grc->delta as well:

net/ipv4/fou_core.c:gue_gro_receive() {
    ...
			guehdr = gue_gro_remcsum(skb, off, guehdr,
						 data + doffset, hdrlen, &grc,
						 !!(fou->flags &
						    FOU_F_REMCSUM_NOPARTIAL));
    ...
}

So is "only set with VXLAN_F_REMCSUM_NOPARTIAL" accurate, or should it
mention FOU_F_REMCSUM_NOPARTIAL too?

>  
>  	ptr = skb_gro_header(skb, plen, grc->offset);

[Severity: Critical]
This is a pre-existing issue and was not introduced by this patch. At
this commit, the non-merged case can still write through a stale frag0.
In udp_gro_receive_segment():

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 release a header-only frags[0] through
__pskb_pull_tail()->skb_frag_unref() without updating frag0 or
frag0_len.

The function can then return NULL on the ip_summed/csum_level mismatch,
or skb_gro_receive_list() can fail after its own pull. In either case
same_flow stays 0, so the new check does not apply.

With the stale frag0_len, skb_gro_header() takes the fast path. Can
remcsum_unadjust() then do a 2-byte read-modify-write into a page the
skb no longer holds a reference on? With page_pool, that page may
already have been recycled into another RX buffer.

The commit message says this case is handled in the following patch.
"net: gro: invalidate frag0 after a pull" appears to add
skb_gro_frag0_invalidate() in three places: before the pskb_may_pull()
calls in udp_gro_receive_segment() and skb_gro_receive_list(), and in
skb_gro_header_slow(). Once that patch is applied, cleanup on a
non-merged skb takes the slow path and writes at
skb->data + grc->offset.

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

[PATCH net v2 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 before 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.

frag0 must be invalidated even if the pull fails: __pskb_pull_tail()
can return NULL after pskb_expand_head() freed the old head, for
instance if a frag_list member has to be cloned or pulled.

Fixes: bd56a29c7a4e ("net: gro: change skb_gro_network_header()")
Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
Cc: stable@vger.kernel.org
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 00b49b6b9e74..6f6affc8ab91 100644
--- a/include/net/gro.h
+++ b/include/net/gro.h
@@ -160,9 +160,19 @@ 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, even if it fails. Call this before the pull, to 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)
 {
+	skb_gro_frag0_invalidate(skb);
 	if (!pskb_may_pull(skb, hlen))
 		return NULL;
 
diff --git a/net/core/gro.c b/net/core/gro.c
index 1b033855a20d..bc88652e353a 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -238,6 +238,7 @@ int skb_gro_receive_list(struct sk_buff *p, struct sk_buff *skb)
 		     NAPI_GRO_CB(skb)->flush))
 		return -E2BIG;
 
+	skb_gro_frag0_invalidate(skb);
 	if (!pskb_may_pull(skb, skb_gro_offset(skb))) {
 		NAPI_GRO_CB(skb)->flush = 1;
 		return -ENOMEM;
diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c
index abd77162f5e7..2309468cfc3d 100644
--- a/net/ipv4/esp4_offload.c
+++ b/net/ipv4/esp4_offload.c
@@ -37,6 +37,7 @@ static struct sk_buff *esp4_gro_receive(struct list_head *head,
 	__be32 seq;
 	__be32 spi;
 
+	skb_gro_frag0_invalidate(skb);
 	if (!pskb_pull(skb, offset))
 		return NULL;
 
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index cf07c3c6611a..cd9d7426b227 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -743,6 +743,7 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
 			pp = p;
 		} else {
 			if (NAPI_GRO_CB(skb)->is_flist) {
+				skb_gro_frag0_invalidate(skb);
 				if (!pskb_may_pull(skb, skb_gro_offset(skb))) {
 					NAPI_GRO_CB(skb)->flush = 1;
 					return NULL;
diff --git a/net/ipv6/esp6_offload.c b/net/ipv6/esp6_offload.c
index 22895521a57d..ca947cb94fb7 100644
--- a/net/ipv6/esp6_offload.c
+++ b/net/ipv6/esp6_offload.c
@@ -64,6 +64,7 @@ static struct sk_buff *esp6_gro_receive(struct list_head *head,
 	if (NAPI_GRO_CB(skb)->proto == IPPROTO_UDP)
 		encap_type = UDP_ENCAP_ESPINUDP;
 
+	skb_gro_frag0_invalidate(skb);
 	if (!pskb_pull(skb, offset))
 		return NULL;
 
-- 
2.53.0

Re: [PATCH net v2 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 (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] net: gro: packet corruption in gro_pull_from_frag0() after
  failed pull

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  `napi_gro_frags()` (net/core/gro.c:783-794) never checks the result of…

--

net: gro: invalidate frag0 after a pull

This patch adds skb_gro_frag0_invalidate(), which clears
NAPI_GRO_CB(skb)->frag0_len. It is called before the slow pulls in
skb_gro_header_slow(), skb_gro_receive_list(), esp4_gro_receive(),
esp6_gro_receive() and udp_gro_receive_segment(). This keeps later GRO
header accesses from using a frag0 pointer that the pull may have
released or reallocated.

> diff --git a/include/net/gro.h b/include/net/gro.h
> index 00b49b6b9e74f..6f6affc8ab91c 100644
> --- a/include/net/gro.h
> +++ b/include/net/gro.h
> @@ -160,9 +160,19 @@ 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, even if it fails. Call this before the pull, to 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)
>  {
> +	skb_gro_frag0_invalidate(skb);
>  	if (!pskb_may_pull(skb, hlen))
>  		return NULL;

[Severity: High]
Does clearing frag0_len here, while frag0 itself is left pointing at
the old location, break gro_pull_from_frag0() when the slow pull in
skb_gro_header_slow() fails?

Consider an skb that enters GRO with skb_headlen(skb) == 0. In that
case skb_gro_reset_offset() points frag0 at the start of frags[0],
which is packet offset 0.

Now add remote checksum offload nopartial: vxlan_gro_receive() or
vxlan_gpe_gro_receive() with VXLAN_F_REMCSUM_NOPARTIAL, or
gue_gro_receive() with FOU_F_REMCSUM_NOPARTIAL. There,
skb_gro_remcsum_process() records grc->offset and grc->delta through
the frag0 fast path. The inner eth_gro_receive() and
inet_gro_receive()/ipv6_gro_receive() handlers then advance
skb_gro_offset() through frag0 too, so skb_headlen() is still 0.

Suppose the next inner header does not fit in frag0_len, for instance
in tcp_gro_pull_header() or udp_gro_udphdr(). skb_gro_header_slow()
now sets frag0_len to 0 and calls pskb_may_pull(). On a truncated
packet where hlen > skb->len, this fails without pulling anything.
That leaves skb_headlen() at 0 and frag0_len at 0.

On the way out, skb_gro_flush_final_remcsum() calls
skb_gro_remcsum_cleanup(), which does:

	ptr = skb_gro_header(skb, plen, grc->offset);

Before this patch that was served from frag0. Now skb_gro_may_pull()
fails, and pskb_may_pull(skb, plen) really copies plen bytes into the
linear area. Here plen = grc->offset + sizeof(u16), which is below
skb_gro_offset(skb) once the inner headers were pulled. frags[0] is
advanced by plen, but NAPI_GRO_CB(skb)->frag0 still points at packet
offset 0.

Since the skb is flushed, dev_gro_receive() ends up in
gro_try_pull_from_frag0(). That computes
grow = skb_gro_offset(skb) - skb_headlen(skb) > 0 and calls
gro_pull_from_frag0():

	memcpy(skb_tail_pointer(skb), NAPI_GRO_CB(skb)->frag0, grow);

This copies packet bytes 0..grow to skb->data + plen, then advances
frags[0] by grow again. The skb handed up the stack as GRO_NORMAL
has its inner headers overwritten with outer header bytes, and the
real bytes at plen..skb_gro_offset() are lost. A remote sender can
trigger this with a short encapsulated packet.

gro_pull_from_frag0() assumes frag0 is the current start of frags[0],
which only holds while skb_headlen() is 0. Should it copy from
skb_frag_address(&pinfo->frags[0]) instead? Or should
skb_gro_remcsum_cleanup() and gro_try_pull_from_frag0() handle a
partially linearized skb in some other way?

[Severity: High]
This patch didn't introduce this, but since the failure path of
skb_gro_header_slow() is being changed here: what happens in
napi_gro_frags() when this pull fails for the Ethernet header?

In net/core/gro.c, napi_frags_skb() takes the slow path and returns
NULL when the pull fails:

net/core/gro.c:napi_frags_skb() {
    ...
	if (unlikely(!skb_gro_may_pull(skb, hlen))) {
		eth = skb_gro_header_slow(skb, hlen, 0);
		if (unlikely(!eth)) {
			net_warn_ratelimited("%s: dropping impossible skb from %s\n",
					     __func__, napi->dev->name);
			napi_reuse_skb(napi, skb);
			return NULL;
    ...
}

The caller doesn't check for NULL, though:

net/core/gro.c:napi_gro_frags() {
	struct sk_buff *skb = napi_frags_skb(napi);

	trace_napi_gro_frags_entry(skb);

	ret = napi_frags_finish(napi, skb, dev_gro_receive(&napi->gro, skb));
    ...
}

dev_gro_receive() starts with skb_get_hash_raw(skb) and then reads
skb->protocol. Can this be a NULL pointer dereference in NAPI softirq
context? One way to reach it would be skb->len < ETH_HLEN, where
pskb_may_pull() fails with PKT_TOO_SMALL. Another would be
__pskb_pull_tail() returning NULL.

napi_gro_frags() used to have an "if (!skb) return GRO_DROP;" check.
Commit 1d11fa696733 ("net-gro: remove GRO_DROP") removed it on the
grounds that NULL could only come from a failed napi_get_frags()
allocation. That reasoning doesn't cover the header pull failure return
shown above.

The drivers checked so far (tun, r8152, e1000) all enforce a minimum
frame length before calling napi_gro_frags(). The other callers have
not been checked.

Should napi_gro_frags() check for a NULL skb again?

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

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

[Eric Dumazet]

Le lun. 2 nov. 2026 à 14:00, <netdev-bot+sashiko@kernel.org> a écrit :
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 (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] net: gro: packet corruption in gro_pull_from_frag0() after
>   failed pull
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but
>   `napi_gro_frags()` (net/core/gro.c:783-794) never checks the result of…
>
> --
>
> net: gro: invalidate frag0 after a pull
>
> This patch adds skb_gro_frag0_invalidate(), which clears
> NAPI_GRO_CB(skb)->frag0_len. It is called before the slow pulls in
> skb_gro_header_slow(), skb_gro_receive_list(), esp4_gro_receive(),
> esp6_gro_receive() and udp_gro_receive_segment(). This keeps later GRO
> header accesses from using a frag0 pointer that the pull may have
> released or reallocated.
>
> > diff --git a/include/net/gro.h b/include/net/gro.h
> > index 00b49b6b9e74f..6f6affc8ab91c 100644
> > --- a/include/net/gro.h
> > +++ b/include/net/gro.h
> > @@ -160,9 +160,19 @@ 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, even if it fails. Call this before the pull, to 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)
> >  {
> > +     skb_gro_frag0_invalidate(skb);
> >       if (!pskb_may_pull(skb, hlen))
> >               return NULL;
>
> [Severity: High]
> Does clearing frag0_len here, while frag0 itself is left pointing at
> the old location, break gro_pull_from_frag0() when the slow pull in
> skb_gro_header_slow() fails?
>
> Consider an skb that enters GRO with skb_headlen(skb) == 0. In that
> case skb_gro_reset_offset() points frag0 at the start of frags[0],
> which is packet offset 0.
>
> Now add remote checksum offload nopartial: vxlan_gro_receive() or
> vxlan_gpe_gro_receive() with VXLAN_F_REMCSUM_NOPARTIAL, or
> gue_gro_receive() with FOU_F_REMCSUM_NOPARTIAL. There,
> skb_gro_remcsum_process() records grc->offset and grc->delta through
> the frag0 fast path. The inner eth_gro_receive() and
> inet_gro_receive()/ipv6_gro_receive() handlers then advance
> skb_gro_offset() through frag0 too, so skb_headlen() is still 0.
>
> Suppose the next inner header does not fit in frag0_len, for instance
> in tcp_gro_pull_header() or udp_gro_udphdr(). skb_gro_header_slow()
> now sets frag0_len to 0 and calls pskb_may_pull(). On a truncated
> packet where hlen > skb->len, this fails without pulling anything.
> That leaves skb_headlen() at 0 and frag0_len at 0.
>
> On the way out, skb_gro_flush_final_remcsum() calls
> skb_gro_remcsum_cleanup(), which does:
>
>         ptr = skb_gro_header(skb, plen, grc->offset);
>
> Before this patch that was served from frag0. Now skb_gro_may_pull()
> fails, and pskb_may_pull(skb, plen) really copies plen bytes into the
> linear area. Here plen = grc->offset + sizeof(u16), which is below
> skb_gro_offset(skb) once the inner headers were pulled. frags[0] is
> advanced by plen, but NAPI_GRO_CB(skb)->frag0 still points at packet
> offset 0.
>
> Since the skb is flushed, dev_gro_receive() ends up in
> gro_try_pull_from_frag0(). That computes
> grow = skb_gro_offset(skb) - skb_headlen(skb) > 0 and calls
> gro_pull_from_frag0():
>
>         memcpy(skb_tail_pointer(skb), NAPI_GRO_CB(skb)->frag0, grow);

Right, this was added in v2 when moving the invalidation before the
pull. After a failed pull, skb_gro_remcsum_cleanup() can pull fewer
bytes than skb_gro_offset(), while frag0 still points to the start
of the packet.

>
> This copies packet bytes 0..grow to skb->data + plen, then advances
> frags[0] by grow again. The skb handed up the stack as GRO_NORMAL
> has its inner headers overwritten with outer header bytes, and the
> real bytes at plen..skb_gro_offset() are lost. A remote sender can
> trigger this with a short encapsulated packet.
>
> gro_pull_from_frag0() assumes frag0 is the current start of frags[0],
> which only holds while skb_headlen() is 0. Should it copy from
> skb_frag_address(&pinfo->frags[0]) instead? Or should
> skb_gro_remcsum_cleanup() and gro_try_pull_from_frag0() handle a
> partially linearized skb in some other way?
>
> [Severity: High]
> This patch didn't introduce this, but since the failure path of
> skb_gro_header_slow() is being changed here: what happens in
> napi_gro_frags() when this pull fails for the Ethernet header?
>
> In net/core/gro.c, napi_frags_skb() takes the slow path and returns
> NULL when the pull fails:
>
> net/core/gro.c:napi_frags_skb() {
>     ...
>         if (unlikely(!skb_gro_may_pull(skb, hlen))) {
>                 eth = skb_gro_header_slow(skb, hlen, 0);
>                 if (unlikely(!eth)) {
>                         net_warn_ratelimited("%s: dropping impossible skb from %s\n",
>                                              __func__, napi->dev->name);
>                         napi_reuse_skb(napi, skb);
>                         return NULL;
>     ...
> }
>
> The caller doesn't check for NULL, though:
>
> net/core/gro.c:napi_gro_frags() {
>         struct sk_buff *skb = napi_frags_skb(napi);
>
>         trace_napi_gro_frags_entry(skb);
>
>         ret = napi_frags_finish(napi, skb, dev_gro_receive(&napi->gro, skb));
>     ...
> }
>
> dev_gro_receive() starts with skb_get_hash_raw(skb) and then reads
> skb->protocol. Can this be a NULL pointer dereference in NAPI softirq
> context? One way to reach it would be skb->len < ETH_HLEN, where
> pskb_may_pull() fails with PKT_TOO_SMALL. Another would be
> __pskb_pull_tail() returning NULL.
>
> napi_gro_frags() used to have an "if (!skb) return GRO_DROP;" check.
> Commit 1d11fa696733 ("net-gro: remove GRO_DROP") removed it on the
> grounds that NULL could only come from a failed napi_get_frags()
> allocation. That reasoning doesn't cover the header pull failure return
> shown above.
>
> The drivers checked so far (tun, r8152, e1000) all enforce a minimum
> frame length before calling napi_gro_frags(). The other callers have
> not been checked.
>
> Should napi_gro_frags() check for a NULL skb again?

I do not think this can happen.

napi_get_frags() allocates the skb with GRO_MAX_HEAD bytes of
tailroom, and these skbs have no frag_list, so __pskb_pull_tail()
can not fail when pulling ETH_HLEN bytes.

pskb_may_pull() can only fail if skb->len < ETH_HLEN, meaning the
driver passed a frame shorter than an Ethernet header, which drivers
must not do. tun, where the length comes from user space, checks it
in tun_get_user().

This is why napi_frags_skb() calls it an "impossible skb". This
could be cleaned up in net-next, but is not a stable fix.

pw-bot: cr