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

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.

[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