[PATCH v3] net: fix wrong network header length

[PATCH v3] 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 different from original,that is difference between ipv6 header and 
ipv4 header. Just change transport_header to be the same with original.

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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 0118f0afaa4f..f5ea977d8f24 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3865,7 +3865,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
 	unsigned int delta_len = 0;
 	struct sk_buff *tail = NULL;
 	struct sk_buff *nskb, *tmp;
-	int err;
+	int len_diff, err;
 
 	skb_push(skb, -skb_network_offset(skb) + offset);
 
@@ -3905,9 +3905,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 v3] net: fix wrong network header length

[Alexei Starovoitov]

On Tue, Feb 15, 2022 at 11:37 PM Lina Wang <lina.wang@mediatek.com> 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.

Please add a test that demonstrates the issue and verifies the fix.

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

[Lina Wang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 1604 bytes --]

On Wed, 2022-02-16 at 19:05 -0800, Alexei Starovoitov wrote:
> On Tue, Feb 15, 2022 at 11:37 PM Lina Wang <lina.wang@mediatek.com>
> 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.
> 
> Please add a test that demonstrates the issue and verifies the fix.

I used iperf udp test to verify the patch, server peer enabled -d to debug
received packets.

192.0.0.4 is clatd interface ip, corresponding ipv6 addr is 
2000:1:1:1:afca:1b1f:1a9:b367, server peer ip is 1.1.1.1,
whose ipv6 is 2004:1:1:1::101:101.

Without the patch, when udp length 2840 packets received, iperf shows:
pcount 1 packet_count 0
pcount 27898727 packet_count 1
pcount 3 packet_count 27898727

pcount should be 2, but is 27898727(0x1a9b367) , which is 20 bytes put 
forward. 

12:08:02.680299	Unicast to us 2004:1:1:1::101:101   2000:1:1:1:afca:1b1f:1a9:b367 UDP 51196 → 5201 Len=2840
0000   20 00 00 01 00 01 00 01 af ca 1b 1f 01 a9 b3 67   ipv6 dst address
0000   c7 fc 14 51 0b 20 c7 ab                           udp header
0000   00 00 00 ab 00 0e f3 49 00 00 00 01 08 06 69 d2   00000001 is pcount
12:08:02.682084	Unicast to us	1.1.1.1	                 192.0.0.4 	 	  UDP 51196 → 5201 Len=2840

After applied the patch, there is no OOO, pcount acted in order.

Thanks!

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

[Paolo Abeni]

Hello,

On Thu, 2022-02-17 at 15:01 +0800, Lina Wang wrote:
> On Wed, 2022-02-16 at 19:05 -0800, Alexei Starovoitov wrote:
> > On Tue, Feb 15, 2022 at 11:37 PM Lina Wang <lina.wang@mediatek.com>
> > 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.
> > 
> > Please add a test that demonstrates the issue and verifies the fix.
> 
> I used iperf udp test to verify the patch, server peer enabled -d to debug
> received packets.
> 
> 192.0.0.4 is clatd interface ip, corresponding ipv6 addr is 
> 2000:1:1:1:afca:1b1f:1a9:b367, server peer ip is 1.1.1.1,
> whose ipv6 is 2004:1:1:1::101:101.
> 
> Without the patch, when udp length 2840 packets received, iperf shows:
> pcount 1 packet_count 0
> pcount 27898727 packet_count 1
> pcount 3 packet_count 27898727
> 
> pcount should be 2, but is 27898727(0x1a9b367) , which is 20 bytes put 
> forward. 
> 
> 12:08:02.680299	Unicast to us 2004:1:1:1::101:101   2000:1:1:1:afca:1b1f:1a9:b367 UDP 51196 → 5201 Len=2840
> 0000   20 00 00 01 00 01 00 01 af ca 1b 1f 01 a9 b3 67   ipv6 dst address
> 0000   c7 fc 14 51 0b 20 c7 ab                           udp header
> 0000   00 00 00 ab 00 0e f3 49 00 00 00 01 08 06 69 d2   00000001 is pcount
> 12:08:02.682084	Unicast to us	1.1.1.1	                 192.0.0.4 	 	  UDP 51196 → 5201 Len=2840
> 
> After applied the patch, there is no OOO, pcount acted in order.

To clarify: Alexei is asking to add a test under:

tools/testing/selftests/net/

to cover this specific case. You can propbably extend the existing
udpgro_fwd.sh.

Please explicitly CC people who gave feedback to previous iterations,
it makes easier to track the discussion.

/P

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

[Alexei Starovoitov]

On Thu, Feb 17, 2022 at 12:45 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Hello,
>
> On Thu, 2022-02-17 at 15:01 +0800, Lina Wang wrote:
> > On Wed, 2022-02-16 at 19:05 -0800, Alexei Starovoitov wrote:
> > > On Tue, Feb 15, 2022 at 11:37 PM Lina Wang <lina.wang@mediatek.com>
> > > 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.
> > >
> > > Please add a test that demonstrates the issue and verifies the fix.
> >
> > I used iperf udp test to verify the patch, server peer enabled -d to debug
> > received packets.
> >
> > 192.0.0.4 is clatd interface ip, corresponding ipv6 addr is
> > 2000:1:1:1:afca:1b1f:1a9:b367, server peer ip is 1.1.1.1,
> > whose ipv6 is 2004:1:1:1::101:101.
> >
> > Without the patch, when udp length 2840 packets received, iperf shows:
> > pcount 1 packet_count 0
> > pcount 27898727 packet_count 1
> > pcount 3 packet_count 27898727
> >
> > pcount should be 2, but is 27898727(0x1a9b367) , which is 20 bytes put
> > forward.
> >
> > 12:08:02.680299       Unicast to us 2004:1:1:1::101:101   2000:1:1:1:afca:1b1f:1a9:b367 UDP 51196 → 5201 Len=2840
> > 0000   20 00 00 01 00 01 00 01 af ca 1b 1f 01 a9 b3 67   ipv6 dst address
> > 0000   c7 fc 14 51 0b 20 c7 ab                           udp header
> > 0000   00 00 00 ab 00 0e f3 49 00 00 00 01 08 06 69 d2   00000001 is pcount
> > 12:08:02.682084       Unicast to us   1.1.1.1                  192.0.0.4                UDP 51196 → 5201 Len=2840
> >
> > After applied the patch, there is no OOO, pcount acted in order.
>
> To clarify: Alexei is asking to add a test under:
>
> tools/testing/selftests/net/
>
> to cover this specific case. You can propbably extend the existing
> udpgro_fwd.sh.

Exactly.
I suspect the setup needs a bit more than just iperf udp test.
Does it need a specific driver and hw?
Can it be reproduced with veth or other virtual netdev?
Also commit talks about bpf_skb_proto_6_to_4.
So that bpf helper has to be somehow involved, but iperf udp test
says nothing about it.
Please craft a _complete_ selftest.

> Please explicitly CC people who gave feedback to previous iterations,
> it makes easier to track the discussion.
>
> /P
>

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

[Lina Wang]

On Thu, 2022-02-17 at 09:05 -0800, Alexei Starovoitov wrote:
> On Thu, Feb 17, 2022 at 12:45 AM Paolo Abeni <pabeni@redhat.com>
> wrote:
> > To clarify: Alexei is asking to add a test under:
> > 
> > tools/testing/selftests/net/
> > 
> > to cover this specific case. You can propbably extend the existing
> > udpgro_fwd.sh.
> 
> Exactly.
> I suspect the setup needs a bit more than just iperf udp test.
> Does it need a specific driver and hw?

My hw is Android phone, Android system has improved clatd with bpf. When 
clatd starts, cls_bpf_classify will handle ingress packets with bpf progs.
bpf_skb_proto_6_to_4 is used here.

> Can it be reproduced with veth or other virtual netdev?

I am not sure. I have no idea how to verify it in linux system. 

> Also commit talks about bpf_skb_proto_6_to_4.
> So that bpf helper has to be somehow involved, but iperf udp test
> says nothing about it.
> Please craft a _complete_ selftest.

Thanks!

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

[Lina Wang]

On Thu, Feb 17, 2022 at 12:45 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Hello,
>
> On Thu, 2022-02-17 at 15:01 +0800, Lina Wang wrote:

> So that bpf helper has to be somehow involved, but iperf udp test
> says nothing about it.
> Please craft a _complete_ selftest.

Finally, I have wrote selftest, please check 
https://lore.kernel.org/bpf/20220407084727.10241-1-lina.wang@mediatek.com/
https://lore.kernel.org/bpf/20220407084727.10241-2-lina.wang@mediatek.com/
https://lore.kernel.org/bpf/20220407084727.10241-3-lina.wang@mediatek.com/ 

Thanks