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 [off-list ref] wrote:

quoted

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/ (local)
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