+ Steffen On Tue, 2022-02-08 at 04:57 -0800, Maciej Żenczykowski wrote: > On Tue, Feb 8, 2022 at 12:25 AM Paolo Abeni <pabeni@xxxxxxxxxx> wrote: > > > > Hello, > > > > On Tue, 2022-02-08 at 10:55 +0800, Lina Wang wrote: > > > When clatd starts with ebpf offloaing, and NETIF_F_GRO_FRAGLIST is enable, > > > several skbs are gathered in skb_shinfo(skb)->frag_list. The first skb's > > > ipv6 header will be changed to ipv4 after bpf_skb_proto_6_to_4, > > > network_header\transport_header\mac_header have been updated as ipv4 acts, > > > but other skbs in frag_list didnot update anything, just ipv6 packets. > > > > > > udp_queue_rcv_skb will call skb_segment_list to traverse other skbs in > > > frag_list and make sure right udp payload is delivered to user space. > > > Unfortunately, other skbs in frag_list who are still ipv6 packets are > > > updated like the first skb and will have wrong transport header length. > > > > > > e.g.before bpf_skb_proto_6_to_4,the first skb and other skbs in frag_list > > > has the same network_header(24)& transport_header(64), after > > > bpf_skb_proto_6_to_4, ipv6 protocol has been changed to ipv4, the first > > > skb's network_header is 44,transport_header is 64, other skbs in frag_list > > > didnot change.After skb_segment_list, the other skbs in frag_list has > > > different network_header(24) and transport_header(44), so there will be 20 > > > bytes difference,that is difference between ipv6 header and ipv4 header. > > > > > Actually, there are two solutions to fix it, one is traversing all skbs > > > and changing every skb header in bpf_skb_proto_6_to_4, the other is > > > modifying frag_list skb's header in skb_segment_list. > > > > I don't think the above should be addressed into the GSO layer. The > > ebpf program is changing the GRO packet in arbitrary way violating the > > GSO packet constraint - arguably, it's corrupting the packet. > > > > I think it would be better change the bpf_skb_proto_6_to_4() to > > properly handle FRAGLIST GSO packets. > > > > If traversing the segments become too costly, you can try replacing > > GRO_FRAGLIST with GRO_UDP_FWD. > > Yeah, I don't know... > > I've considered that we could perhaps fix the 6to4 helper, and 4to6 helper... > but then I think every *other* helper / code path that plays games > with the packet header needs fixing as well, > ie. everything dealing with encap/decap, vlan, etc.. > > At that point it seems to me like it's worth fixing here rather than > in all those other places. > > In general it seems gro fraglist as implemented is just a bad idea... > Packets (and things we treat like packets) really should only have 1 header. > GRO fraglist - as implemented - violates this pretty fundamental assumption. > As such it seems to be on the gro fraglist implementation to deal with it. > That to me seems to mean it should be fixed here, and not elsewhere. @Steffen: IIRC GRO_FRAGLIST was originally added to support some forwarding scenarios. Now we have GRO_UDP_FWD which should be quite comparable. I'm wondering if the latter feature addresses your use case, too. If so, could we consider deprecating (and in a longer run, drop) the GRO_FRAGLIST feature? Thanks! Paolo
