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

[netdev-bot+sashiko@kernel.org]

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