[PATCH] net: fix wrong network header length

[PATCH] net: fix wrong network header length

[Lina Wang]

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. Considering
efficiency, adopt the second one--- when the first skb and other skbs in
frag_list has different network_header length, restore them to make sure
right udp payload is delivered to user space.


Signed-off-by: Lina Wang <lina.wang@mediatek.com>

---
 net/core/skbuff.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 75dfbde8d2e6..f15bbb7449ce 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3682,6 +3682,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
 	struct sk_buff *tail = NULL;
 	struct sk_buff *nskb, *tmp;
 	int err;
+	unsigned int len_diff = 0;
 
 	skb_push(skb, -skb_network_offset(skb) + offset);
 
@@ -3721,9 +3722,11 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
 		skb_push(nskb, -skb_network_offset(nskb) + offset);
 
 		skb_release_head_state(nskb);
+		len_diff = skb_network_header_len(nskb) - skb_network_header_len(skb);
 		 __copy_skb_header(nskb, skb);
 
 		skb_headers_offset_update(nskb, skb_headroom(nskb) - skb_headroom(skb));
+		nskb->transport_header += len_diff;
 		skb_copy_from_linear_data_offset(skb, -tnl_hlen,
 						 nskb->data - tnl_hlen,
 						 offset + tnl_hlen);
-- 
2.18.0

Re: [PATCH] net: fix wrong network header length

[Paolo Abeni]

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.

Thanks!

Paolo

Re: [PATCH] net: fix wrong network header length

[Maciej Żenczykowski]

On Tue, Feb 8, 2022 at 12:25 AM Paolo Abeni <pabeni@redhat.com> 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)

Re: [PATCH] net: fix wrong network header length

[Paolo Abeni]

+ 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@redhat.com> 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

Re: [PATCH] net: fix wrong network header length

[Jakub Kicinski]

On Wed, 9 Feb 2022 18:25:07 +0800 lina.wang wrote:
> We use NETIF_F_GRO_FRAGLIST not for forwarding scenary, just for
> software udp gro. Whatever NETIF_F_GRO_FRAGLIST or NETIF_F_GRO_FWD,
> skb_segment_list should not have bugs.
> 
> We modify skb_segment_list, not in epbf. One point is traversing the
> segments costly, another is what @Maciej said, *other* helper may have
> the same problem. In skb_segment_list, it calls
> skb_headers_offset_update to update different headroom, which implys
> header maybe different.

Process notes:
 - the patch didn't apply so even if the discussion concludes that 
   the patch was good you'll need to rebase on netdev/net and repost;
 - please don't top post.

Re: [PATCH] net: fix wrong network header length

[Steffen Klassert]

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.

Re: [PATCH] net: fix wrong network header length

[Lina Wang]

From: lina.wang <lina.wang@mediatek.com>

On Wed, 2022-02-09 at 17:06 -0800, Jakub Kicinski wrote:
> On Wed, 9 Feb 2022 18:25:07 +0800 lina.wang wrote:
> > We use NETIF_F_GRO_FRAGLIST not for forwarding scenary, just for
> > software udp gro. Whatever NETIF_F_GRO_FRAGLIST or NETIF_F_GRO_FWD,
> > skb_segment_list should not have bugs.
> > 
> > We modify skb_segment_list, not in epbf. One point is traversing
> > the
> > segments costly, another is what @Maciej said, *other* helper may
> > have
> > the same problem. In skb_segment_list, it calls
> > skb_headers_offset_update to update different headroom, which
> > implys
> > header maybe different.
> 
> Process notes:
>  - the patch didn't apply so even if the discussion concludes that 
>    the patch was good you'll need to rebase on netdev/net and repost;
>  - please don't top post.

I have rebased and reposted--
https://patchwork.kernel.org/project/netdevbpf/patch/20220210091655.17231-1-lina.wang@mediatek.com/, 
please check if it can apply.

Thanks!

Re: [PATCH] net: fix wrong network header length

[Paolo Abeni]

On Wed, 2022-02-09 at 18:25 +0800, lina.wang wrote:
> We use NETIF_F_GRO_FRAGLIST not for forwarding scenary, just for
> software udp gro. 
> 
I'm wondering why don't you simply enable UDP_GRO on the relevant
socket? 

> Whatever NETIF_F_GRO_FRAGLIST or NETIF_F_GRO_FWD,
> skb_segment_list should not have bugs.

The bug is arguably in bpf_skb_proto_6_to_4(), even if fixing it in
skb_segment_list() is possibly easier.

> We modify skb_segment_list, not in epbf. One point is traversing the
> segments costly, another is what @Maciej said, *other* helper may have
> the same problem. In skb_segment_list, it calls
> skb_headers_offset_update to update different headroom, which implys
> header maybe different.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 75dfbde8d2e6..f15bbb7449ce 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3682,6 +3682,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
>  	struct sk_buff *tail = NULL;
>  	struct sk_buff *nskb, *tmp;
>  	int err;
> +	unsigned int len_diff = 0;

Mintor nit: please respect the reverse x-mas tree order.

>  
>  	skb_push(skb, -skb_network_offset(skb) + offset);

> @@ -3721,9 +3722,11 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
>  		skb_push(nskb, -skb_network_offset(nskb) + offset);
>  
>  		skb_release_head_state(nskb);
> +		len_diff = skb_network_header_len(nskb) - skb_network_header_len(skb);
>  		 __copy_skb_header(nskb, skb);
>  
>  		skb_headers_offset_update(nskb, skb_headroom(nskb) - skb_headroom(skb));
> +		nskb->transport_header += len_diff;

This does not look correct ?!? the network hdr position for nskb will
still be uncorrect?!? and even the mac hdr likely?!? possibly you need
to change the offset in skb_headers_offset_update().

Paolo

Re: [PATCH] net: fix wrong network header length

[Maciej Żenczykowski]

> I'm wondering why don't you simply enable UDP_GRO on the relevant
> socket?

Oh this is simple.
We don't control the socket.  Apps do.  ie. the entirety of the rest
of the internet.

Re: [PATCH] net: fix wrong network header length

[Lina Wang]

On Thu, 2022-02-10 at 17:02 +0100, Paolo Abeni wrote:

> > @@ -3682,6 +3682,7 @@ struct sk_buff *skb_segment_list(struct
> > sk_buff *skb,
> >  	struct sk_buff *tail = NULL;
> >  	struct sk_buff *nskb, *tmp;
> >  	int err;
> > +	unsigned int len_diff = 0;
> 
> Mintor nit: please respect the reverse x-mas tree order.
> 

Yes,v2 has change unsigned int to int

> >  
> >  	skb_push(skb, -skb_network_offset(skb) + offset);
> > @@ -3721,9 +3722,11 @@ struct sk_buff *skb_segment_list(struct
> > sk_buff *skb,
> >  		skb_push(nskb, -skb_network_offset(nskb) + offset);
> >  
> >  		skb_release_head_state(nskb);
> > +		len_diff = skb_network_header_len(nskb) -
> > skb_network_header_len(skb);
> >  		 __copy_skb_header(nskb, skb);
> >  
> >  		skb_headers_offset_update(nskb, skb_headroom(nskb) -
> > skb_headroom(skb));
> > +		nskb->transport_header += len_diff;
> 
> This does not look correct ?!? the network hdr position for nskb will
> still be uncorrect?!? and even the mac hdr likely?!? possibly you
> need
> to change the offset in skb_headers_offset_update().
> 

Network hdr position and mac hdr are both right, because bpf processing & 
skb_headers_offset_update have updated them to right position. After bpf
loading, the first skb's network header&mac_header became 44, transport
header still is 64. After skb_headers_offset_update, fraglist skb's mac
header and network header are still 24, the same with original packet.
Just fraglist skb's transport header became 44, as original is 64. 
Only transport header cannot be easily updated the same offset, because 
6to4 has different network header.

Actually,at the beginning, I want to change skb_headers_offset_update, but 
it has been called also in other place, maybe a new function should be 
needed here.
 
Skb_headers_offset_update has other wrong part in my scenary, 
inner_transport_header\inner_network_header\inner_mac_header shouldnot be 
changed, but they are been updated because of different headroom. They are
not used later, so wrong value didnot affect anything.

> Paolo
>

Thanks! 

Re: [PATCH] net: fix wrong network header length

[Maciej Żenczykowski]

On Thu, Feb 10, 2022 at 8:12 PM Lina Wang <lina.wang@mediatek.com> wrote:
>
> On Thu, 2022-02-10 at 17:02 +0100, Paolo Abeni wrote:
>
> > > @@ -3682,6 +3682,7 @@ struct sk_buff *skb_segment_list(struct
> > > sk_buff *skb,
> > >     struct sk_buff *tail = NULL;
> > >     struct sk_buff *nskb, *tmp;
> > >     int err;
> > > +   unsigned int len_diff = 0;
> >
> > Mintor nit: please respect the reverse x-mas tree order.
> >
>
> Yes,v2 has change unsigned int to int

Reverse christmas tree, means from longest to shortest, like so:

     struct sk_buff *tail = NULL;
     struct sk_buff *nskb, *tmp;
+   int len_diff = 0;
     int err;

That said, I think the = 0 is not needed, so this can be just

+ int len_diff, err;

>
> > >
> > >     skb_push(skb, -skb_network_offset(skb) + offset);
> > > @@ -3721,9 +3722,11 @@ struct sk_buff *skb_segment_list(struct
> > > sk_buff *skb,
> > >             skb_push(nskb, -skb_network_offset(nskb) + offset);
> > >
> > >             skb_release_head_state(nskb);
> > > +           len_diff = skb_network_header_len(nskb) -
> > > skb_network_header_len(skb);
> > >              __copy_skb_header(nskb, skb);
> > >
> > >             skb_headers_offset_update(nskb, skb_headroom(nskb) -
> > > skb_headroom(skb));
> > > +           nskb->transport_header += len_diff;
> >
> > This does not look correct ?!? the network hdr position for nskb will
> > still be uncorrect?!? and even the mac hdr likely?!? possibly you
> > need
> > to change the offset in skb_headers_offset_update().
> >
>
> Network hdr position and mac hdr are both right, because bpf processing &
> skb_headers_offset_update have updated them to right position. After bpf
> loading, the first skb's network header&mac_header became 44, transport
> header still is 64. After skb_headers_offset_update, fraglist skb's mac
> header and network header are still 24, the same with original packet.
> Just fraglist skb's transport header became 44, as original is 64.
> Only transport header cannot be easily updated the same offset, because
> 6to4 has different network header.
>
> Actually,at the beginning, I want to change skb_headers_offset_update, but
> it has been called also in other place, maybe a new function should be
> needed here.
>
> Skb_headers_offset_update has other wrong part in my scenary,
> inner_transport_header\inner_network_header\inner_mac_header shouldnot be
> changed, but they are been updated because of different headroom. They are
> not used later, so wrong value didnot affect anything.
>
> > Paolo
> >
>
> Thanks!Maciej Żenczykowski, Kernel Networking Developer @ Google

Re: [PATCH] net: fix wrong network header length

[Lina Wang]

On Thu, 2022-02-10 at 17:02 +0100, Paolo Abeni wrote:
> On Wed, 2022-02-09 at 18:25 +0800, lina.wang wrote:
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 75dfbde8d2e6..f15bbb7449ce 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -3682,6 +3682,7 @@ struct sk_buff *skb_segment_list(struct
> > sk_buff *skb,
> >  	struct sk_buff *tail = NULL;
> >  	struct sk_buff *nskb, *tmp;
> >  	int err;
> > +	unsigned int len_diff = 0;
> 
> Mintor nit: please respect the reverse x-mas tree order.
> 

v3 has updated, please give some advice!

> >  
> Paolo
> 

Thanks!