On Tue, Feb 8, 2022 at 12:25 AM Paolo Abeni <[email protected]> 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.
(btw. wrt. this commit itself, it seems like the diff should be a signed int)
+ Steffen
On Tue, 2022-02-08 at 04:57 -0800, Maciej Żenczykowski wrote:
> On Tue, Feb 8, 2022 at 12:25 AM Paolo Abeni <[email protected]> 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
On Tue, Feb 08, 2022 at 05:01:07PM +0100, Paolo Abeni wrote:
> + Steffen
> On Tue, 2022-02-08 at 04:57 -0800, Maciej Żenczykowski wrote:
> > >
> > > 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.
The advantage of GRO_FRAGLIST for forwarding is that GRO and GSO
happen with almost no overhead, because the packets are left in
the skbs we received them and are not mangled during processing.
So if there is no hardware segmentation support, GRO_FRAGLIST is
still much faster than GRO_UDP_FWD.
> If so, could we consider deprecating (and in a longer run, drop) the
> GRO_FRAGLIST feature?
Maybe we can make it exclusive for forwarding or bring the header
processing a bit closer to GRO_UDP_FWD, but I'd like to keep that
feature.