netdev - Re: Broken segmentation on UDP_GSO / VIRTIO_NET_HDR_GSO_UDP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 12 Jul 2023 17:19:32 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Yan Zhai <yan@...udflare.com>, Marek Majkowski <marek@...udflare.com>
Cc: network dev <netdev@...r.kernel.org>, kernel-team
 <kernel-team@...udflare.com>, Andrew Melnychenko <andrew@...nix.com>, Jason
 Wang <jasowang@...hat.com>
Subject: Re: Broken segmentation on UDP_GSO / VIRTIO_NET_HDR_GSO_UDP_L4
 forwarding path

Adding Jason,
On Wed, 2023-07-12 at 09:58 -0500, Yan Zhai wrote:
> On Wed, Jul 12, 2023 at 9:00 AM Marek Majkowski <marek@...udflare.com> wrote:
> > 
> > Dear netdev,
> > 
> > I encountered a puzzling problem, please help.
> > 
> > Rootless repro:
> >    https://gist.github.com/majek/5e8fd12e7a1663cea63877920fe86f18
> > 
> > To run:
> > 
> > ```
> > $ unshare -Urn python3 udp-gro-forwarding-bug.py
> > tap0  In  IP 10.0.0.2.55892 > 1.1.1.1.5021: UDP, length 4000
> > lo    P   IP 10.0.0.2.55892 > 1.1.1.1.5021: UDP, bad length 4000 > 1392
> > lo    P   IP 10.0.0.2.43690 > 1.1.1.1.43690: UDP, bad length 43682 > 1392
> > lo    P   IP 10.0.0.2.43690 > 1.1.1.1.43690: UDP, bad length 43682 > 1200
> > '''
> > 
> > The code is really quite simple. First I create and open a tap device.
> > Then I send a large (>MTU) packet with vnethdr over tap0. The
> > gso_type=GSO_UDP_L4, and gso_size=1400. I expect the packet to egress
> > from tap0, be forwarded somewhere, where it will eventually be
> > segmented by software or hardware.
> > 
> > The egress tap0 packet looks perfectly fine:
> > 
> > tap0  In  IP 10.0.0.2.55892 > 1.1.1.1.5021: UDP, length 4000
> > 
> > To simplify routing I'm doing 'tc mirred' aka `bpf_redirect()` magic,
> > where I move egress tap0 packets to ingress lo, like this:
> > 
> > > tc filter add dev tap0 ingress protocol ip u32 match ip src 10.0.0.2 action mirred egress redirect dev lo
> > 
> > On ingress lo I see something really weird:
> > 
> > lo    P   IP 10.0.0.2.55892 > 1.1.1.1.5021: UDP, bad length 4000 > 1392
> > lo    P   IP 10.0.0.2.43690 > 1.1.1.1.43690: UDP, bad length 43682 > 1392
> > lo    P   IP 10.0.0.2.43690 > 1.1.1.1.43690: UDP, bad length 43682 > 1200
> > 
> > This looks like IPv4 fragments without the IP fragmentation bits set.
> > 
> > I think there are two independent problems here:
> > 
> > (1) The packet is *fragmented* and that is plain wrong here. I'm
> > asking for USO not UFO in vnethdr.
> > 
> 
> To add some context our virtio header in hex format (12 bytes) is
> 01052a007805220006000000.
> 
> Some digging shows that the issue seems to come from this patch:
> https://lore.kernel.org/netdev/20220907125048.396126-2-andrew@daynix.com/
> At this point, skb_shared_info->gso_type is SKB_GSO_UDP_L4 |
> SKB_GSO_DODGY, here the DODGY bit is set inside tun_get_user. So the
> skb_gso_ok check will return true here, then the skb will fall to the
> fragment code. Simple tracing confirms that __udp_gso_segment is never
> called in this scenario.
> 
> So the question is really how to handle the DODGY bit. IMHO it is not
> right to fall to the fragment path when the actual packet request is
> segmentation. 

I do agree with the above.

> Will it be sufficient to just recompute the gso_segs
> here and return the head skb instead?

Something alike what TCP is currently doing:

https://elixir.bootlin.com/linux/latest/source/net/ipv4/tcp_offload.c#L85

should be fine - constrained to the skb_shinfo(skb)->gso_type &
SKB_GSO_UDP_L4 case.

Cheers,

Paolo