Felix Fietkau wrote:
On 26.09.24 09:19, Willem de Bruijn wrote:
Felix Fietkau wrote:
On 25.09.24 22:59, Felix Fietkau wrote:
On 25.09.24 21:09, Willem de Bruijn wrote:
Felix Fietkau wrote:
On 22.09.24 17:03, Willem de Bruijn wrote: > 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... > 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.