Felix Fietkau wrote:

On 26.09.24 09:19, Willem de Bruijn wrote:

quoted

Felix Fietkau wrote:

quoted

On 25.09.24 22:59, Felix Fietkau wrote:

quoted

On 25.09.24 21:09, Willem de Bruijn wrote:

quoted

Felix Fietkau wrote:

quoted

On 22.09.24 17:03, Willem de Bruijn wrote:

quoted

From: Willem de Bruijn <willemb@google.com>

Detect gso fraglist skbs with corrupted geometry (see below) and
pass these to skb_segment instead of skb_segment_list, as the first
can segment them correctly.

Valid SKB_GSO_FRAGLIST skbs
- consist of two or more segments
- the head_skb holds the protocol headers plus first gso_size
- one or more frag_list skbs hold exactly one segment
- all but the last must be gso_size

Optional datapath hooks such as NAT and BPF (bpf_skb_pull_data) can
modify these skbs, breaking these invariants.

In extreme cases they pull all data into skb linear. For UDP, this
causes a NULL ptr deref in __udpv4_gso_segment_list_csum at
udp_hdr(seg->next)->dest.

Detect invalid geometry due to pull, by checking head_skb size.
Don't just drop, as this may blackhole a destination. Convert to be
able to pass to regular skb_segment.

Link: https://lore.kernel.org/netdev/20240428142913.18666-1-shiming.cheng@mediatek.com/ (local)
Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
Signed-off-by: Willem de Bruijn <willemb@google.com>
Cc: stable@vger.kernel.org

---

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index d842303587af..e457fa9143a6 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c

@@ -296,8 +296,16 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
  		return NULL;
  	}
  
-	if (skb_shinfo(gso_skb)->gso_type & SKB_GSO_FRAGLIST)
-		return __udp_gso_segment_list(gso_skb, features, is_ipv6);
+	if (skb_shinfo(gso_skb)->gso_type & SKB_GSO_FRAGLIST) {
+		 /* Detect modified geometry and pass these to skb_segment. */
+		if (skb_pagelen(gso_skb) - sizeof(*uh) == skb_shinfo(gso_skb)->gso_size)
+			return __udp_gso_segment_list(gso_skb, features, is_ipv6);
+
+		 /* Setup csum, as fraglist skips this in udp4_gro_receive. */
+		gso_skb->csum_start = skb_transport_header(gso_skb) - gso_skb->head;
+		gso_skb->csum_offset = offsetof(struct udphdr, check);
+		gso_skb->ip_summed = CHECKSUM_PARTIAL;

I also noticed this uh->check update done by udp4_gro_complete only in 
case of non-fraglist GRO:

     if (uh->check)
         uh->check = ~udp_v4_check(skb->len - nhoff, iph->saddr,
                       iph->daddr, 0);

I didn't see any equivalent in your patch. Is it missing or left out 
intentionally?

Thanks. That was not intentional. I think you're right. Am a bit
concerned that all this testing did not catch it. Perhaps because
CHECKSUM_PARTIAL looped to ingress on the same machine is simply
interpreted as CHECKSUM_UNNECESSARY. Need to look into that.

If respinning this, I should also change the Fixes to

Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")

Analogous to the eventual TCP fix to

Fixes: bee88cd5bd83 ("net: add support for segmenting TCP fraglist GSO packets")

In the mean time, I've been working on the TCP side. I managed to
reproduce the issue on one of my devices by routing traffic from
Ethernet to Wifi using your BPF test program.

The following patch makes it work for me for TCP v4. Still need to
test and fix v6.

Actually, here is something even simpler that should work for both v4
and v6:

Makes sense. It does come with higher cost of calling skb_checksum.

But only if there is no checksum offload, right? Because the way I 
implemented it, the lines further below starting with
if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL))
will initialize th->check and set ip_summed to CHECKSUM_PARTIAL
Or am I missing something?

Oh you're right, __tcp_v4_send_check then does this, so no need to
open code it.