Re: [ovs-dev] [PATCH 1/8] ip6_gre: Fix a bug that clears address bits

[Yifeng Sun]

Hi Greg,

Thanks for clarifying this bug, you did a much better job than I did. Sorry
I didn't describe it clearly.

I will created another patch, as you suggested.


William, below is the crash dump you requested.

[  645.832991] general protection fault:  [#1] SMP PTI
[  645.833033] Modules linked in: veth vport_vxlan(OE) vport_stt(OE)
vport_lisp(OE) vport_geneve(OE) openvswitch(OE) tunnel6 nf_conntrack_ipv6
nf_defrag_ipv6 nf_nat_ipv6 nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4
nf_nat nf_conntrack udp_tunnel netconsole rpcsec_gss_krb5 auth_rpcgss nfsv4
nfs vmw_vsock_vmci_transport vsock lockd grace fscache vmw_balloon sb_edac
joydev input_leds serio_raw coretemp vmw_vmci shpchp i2c_piix4 mac_hid
ib_iser rdma_cm iw_cm ib_cm ib_core sunrpc configfs iscsi_tcp libiscsi_tcp
libiscsi scsi_transport_iscsi autofs4 btrfs zstd_decompress zstd_compress
xxhash raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor
async_tx xor raid6_pq libcrc32c raid1 raid0 multipath linear hid_generic
usbhid hid crct10dif_pclmul crc32_pclmul ghash_clmulni_intel vmwgfx pcbc
ttm drm_kms_helper
[  645.836096]  syscopyarea sysfillrect sysimgblt fb_sys_fops drm psmouse
aesni_intel aes_x86_64 crypto_simd glue_helper cryptd mptspi mptscsih
mptbase vmxnet3 ahci libahci scsi_transport_spi pata_acpi
[  645.837697] CPU: 0 PID: 17724 Comm: handler2 Tainted: G   OE
 4.15.18 #1
[  645.838250] Hardware name: VMware, Inc. VMware Virtual Platform/440BX
Desktop Reference Platform, BIOS 6.00 09/21/2015
[  645.839419] RIP: 0010:ip6erspan_tunnel_xmit+0x1da/0x6f0 [openvswitch]
[  645.839996] RSP: 0018:b36d008177c0 EFLAGS: 00010286
[  645.840562] RAX: ffea RBX: 9be571e84000 RCX:
0040
[  645.841126] RDX: 003e RSI: 71cf7e00 RDI:
9be571cf7600
[  645.841697] RBP: b36d00817880 R08: 0018 R09:

[  645.842268] R10: b36d00817a90 R11: fffe R12:
9be5610e4400
[  645.842838] R13:  R14: 0001 R15:

[  645.843422] FS:  7eff6e146700() GS:9be5bfc0()
knlGS:
[  645.844015] CS:  0010 DS:  ES:  CR0: 80050033
[  645.844607] CR2: 7febc8f859d0 CR3: 5de8a002 CR4:
001606f0
[  645.845250] Call Trace:
[  645.846070]  do_execute_actions+0x1505/0x1900 [openvswitch]
[  645.846686]  ? reserve_sfa_size+0x28/0x100 [openvswitch]
[  645.847319]  ? __ovs_nla_copy_actions+0x15c/0x6d0 [openvswitch]
[  645.847958]  ? __kmalloc_reserve.isra.39+0x2e/0x80
[  645.848581]  ? _cond_resched+0x16/0x40
[  645.849196]  ? __kmalloc+0x16d/0x1f0
[  645.849791]  ? ovs_execute_actions+0x48/0x120 [openvswitch]
[  645.850401]  ovs_execute_actions+0x48/0x120 [openvswitch]
[  645.851012]  ovs_packet_cmd_execute+0x267/0x2b0 [openvswitch]
[  645.851624]  genl_family_rcv_msg+0x1e9/0x380
[  645.852231]  genl_rcv_msg+0x47/0x90
[  645.852820]  ? genl_family_rcv_msg+0x380/0x380
[  645.853399]  netlink_rcv_skb+0xde/0x110
[  645.853962]  genl_rcv+0x24/0x40
[  645.854505]  netlink_unicast+0x183/0x240
[  645.855040]  netlink_sendmsg+0x2c2/0x3b0
[  645.855568]  sock_sendmsg+0x36/0x40
[  645.856074]  ___sys_sendmsg+0x2bc/0x2d0
[  645.856571]  ? ep_item_poll.isra.10+0x3b/0xc0
[  645.857045]  ? ep_send_events_proc+0x87/0x1a0
[  645.857501]  ? ep_read_events_proc+0xd0/0xd0
[  645.857940]  ? ep_scan_ready_list+0x1f3/0x200
[  645.858371]  ? ep_poll+0x1fb/0x3b0
[  645.858788]  ? __sys_sendmsg+0x51/0x90
[  645.859189]  __sys_sendmsg+0x51/0x90
[  645.859585]  do_syscall_64+0x6e/0x120
[  645.859964]  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
[  645.860472] RIP: 0033:0x7eff6ec36a6d
[  645.860843] RSP: 002b:7eff6e0e81a0 EFLAGS: 0293 ORIG_RAX:
002e
[  645.861226] RAX: ffda RBX: 0002 RCX:
7eff6ec36a6d
[  645.861622] RDX:  RSI: 7eff6e0e8200 RDI:
0011
[  645.862009] RBP: 7eff6e0e8fe0 R08:  R09:
7eff6e0e9a60
[  645.862396] R10: 00a5d340 R11: 0293 R12:
009f7690
[  645.862775] R13: 7eff6e0e8200 R14: 0002 R15:
7eff6e0e86a0
[  645.863189] Code: 0f 85 a6 fe ff ff 45 31 c9 b8 ea ff ff ff 66 45 89 4c
24 3c 66 81 a3 1a 09 00 00 ff fb 49 8b 74 24 38 48 85 f6 0f 84 a2 fe ff ff
<0f> b6 96 f1 00 00 00 b8 ea ff ff ff f6 c2 01 0f 84 8d fe ff ff
[  645.864513] RIP: ip6erspan_tunnel_xmit+0x1da/0x6f0 [openvswitch] RSP:
b36d008177c0
[  645.865043] ---[ end trace 25bab82318677b6f ]---
[  645.865452] Kernel panic - not syncing: Fatal exception in interrupt
[  645.865923] Kernel Offset: 0x3d00 from 0x8100
(relocation range: 0x8000-0xbfff)
[  645.866752] ---[ end Kernel panic - not syncing: Fatal exception in
interrupt


On Thu, Aug 9, 2018 at 3:59 PM, Gregory Rose  wrote:

> On 8/8/2018 11:32 AM, Yifeng Sun wrote:
>
>> In compatable gre module, skb->cb is used as ovs_gso_cb.
>> This bug clears the 16-23 bit in the address of ovs

Re: [ovs-dev] [PATCH 1/8] ip6_gre: Fix a bug that clears address bits

[Gregory Rose]

On 8/8/2018 11:32 AM, Yifeng Sun wrote:

In compatable gre module, skb->cb is used as ovs_gso_cb.
This bug clears the 16-23 bit in the address of ovs_gso_cb.tun_dst.

Signed-off-by: Yifeng Sun 
---
  datapath/linux/compat/ip6_gre.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/datapath/linux/compat/ip6_gre.c b/datapath/linux/compat/ip6_gre.c
index 54a76ab..16c1f72 100644
--- a/datapath/linux/compat/ip6_gre.c
+++ b/datapath/linux/compat/ip6_gre.c
@@ -1146,7 +1146,6 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff 
*skb,
goto tx_err;
  
  	t->parms.o_flags &= ~TUNNEL_KEY;

-   IPCB(skb)->flags = 0;
  
  	tun_info = ovs_skb_tunnel_info(skb);

if (unlikely(!tun_info ||


Yifeng,

First, I'm sorry but you actually did identify the problem correctly at 
first, I just didn't see it.  My bad.


Second, I'm no longer worried about removing the code.  It shouldn't be 
used in the case where
we are not using USE_UPSTREAM_TUNNEL.  If USE_UPSTREAM_TUNNEL is defined 
then this entire
module does not compile anyway.  So I think removing the IPCB macro that 
sets the flags to zero is fine.


The same needs to be done in _gre6_xmit().

Go ahead and resubmit the patch with the _gre6_xmit() case handled as 
well and I think that should be fine.


Thanks for all your work!!!

- Greg
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/8] ip6_gre: Fix a bug that clears address bits

[Gregory Rose]

On 8/9/2018 3:33 PM, William Tu wrote:



On Thu, Aug 9, 2018 at 2:37 PM, Gregory Rose > wrote:


On 8/9/2018 11:36 AM, Yifeng Sun wrote:

Yes, I agree. It should be the way to go.

Thanks,
Yifeng



If I'm reading the code correctly the OVS packet cmd will set the
skb->cb using the OVS_CB macro and then
pass that packet down to the ip6erspan_tunnel_xmit function.  The
ip6erspan_tunnel_xmit function will
then use the IPCB macro to access the skb->cb area to set the flag
to zero.  That works fine for packets
generated by the system that have used the IPCB macro accessor to
set the skb->cb data.  However, it
is catastrophic when an skb was prepared with the OVS_CB macro
because, as you might imagine, it
is not pointing to an inet_skb_parm structure but an ovs_skb_cb
structure instead.

on thing I don't understand is that:
when packet/skb is sent to ip6erspan_tunnel_xmit, it is already out of 
OVS code.
ip6erspan_tunnel_xmit clears the inner skb->cb, encap skb with outer 
erspan+gre header,
then lookup the next netdev using outer ip address. The skb again 
arrives at another netdev.
Until this point, there is no OVS code involves and so OVS_CB macro 
shouldn't cause issue.


Yifeng, can you share the crash dump?
-- William


OK, I did not read the code correctly.  It's an ovs_gso_cb structure and 
now I get where you were talking
about bits 16-23 being over written - that being the tun_dst pointer in 
our case since USE_UPSTREAM_TUNNEL

is not defined.  That wasn't really clear to me before.

Here's a before and after dump of the skb->cb data area:
[17182.462235] Before -
[17182.463006] 00 9c 6f 8b 21 9d ff ff 00 00 4c 00 00 00 00 00
[17182.463010]00 f2 22 b6 21 9d ff ff 00 00 00 00 00 00 00 00
[17182.463777] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[17182.464519] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[17182.465277] -
[17182.466749] After -
[17182.467462] 00 9c 6f 8b 21 9d ff ff 00 00 4c 00 00 00 00 00
[17182.467465] 00 f2 22 b6 00 00 ff ff 00 00 00 00 00 00 00 00
[17182.468221] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[17182.468968] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[17182.469734] -

Note the bits 16-23 of the tun_dst address being overwritten...  :O

Guh - keeping track of all the  CB accessors is a real tangled mess.

- Greg



This code was *never* correct.  Somehow or other it didn't cause a
problem until 4.15.  We'll have to
figure out some way to check if the packet is ours or is system
generated to know if we should be using
the IPCB accessor.

In instances of packets generated by the system or any other
entity transmitting ipv6 packets over the
tunnel we'll need to keep the code that clears the flags.

Thanks,

- Greg



On Thu, Aug 9, 2018 at 11:17 AM, Gregory Rose
mailto:gvrose8...@gmail.com>> wrote:

On 8/9/2018 11:03 AM, Yifeng Sun wrote:

There is a difference regarding how skb_tunnel_info is
stored in skb
between ovs and upstream kernel. In ovs's compatible gre module,
skb_tunnel_info is stored in skb->cb while in upstream
kernel, it is
referenced by skb->_skb_refdst.

The upstream netdev code should be okay.

To fix this issue, my guess is that either we comply to the
kernel's way
by using skb->_skb_refdst to store skb_tunnel_info, or we
don't use
IPCB at all.


Ah, I see...

We must comply with the kernel method for any given kernel. 
We can't be sure that we'll only handle
or own packets.

Can't this be fixed by a compatibility layer #define in
acinclude.m4 so that kernels that store it in
skb->cb vs. kernels that store it in skb->__skb_refdst will
both work?

Thanks,

- Greg




Thanks,
Yifeng



On Thu, Aug 9, 2018 at 10:28 AM, Gregory Rose
mailto:gvrose8...@gmail.com>> wrote:


On 8/8/2018 6:50 PM, William Tu wrote:

thanks for the fix.

On Wed, Aug 8, 2018 at 11:32 AM, Yifeng Sun
mailto:pkusunyif...@gmail.com>> wrote:

In compatable gre module, skb->cb is used as
ovs_gso_cb.
This bug clears the 16-23 bit in the address of
ovs_gso_cb.tun_dst.

can you explain more about ovs_gso_cb?

Signed-off-by: Yifeng Sun
mailto:pkusunyif...@gmail.com>>
---
datapath/linux/compat/ip6_gre.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/datapath/linux/compat/ip6_gre.c
b/datapath/linux/compat/ip6_
gre.c
index 54a76ab..16c1f72 100644
  

Re: [ovs-dev] [PATCH 1/8] ip6_gre: Fix a bug that clears address bits

[William Tu]

On Thu, Aug 9, 2018 at 2:37 PM, Gregory Rose  wrote:

> On 8/9/2018 11:36 AM, Yifeng Sun wrote:
>
> Yes, I agree. It should be the way to go.
>
> Thanks,
> Yifeng
>
>
> If I'm reading the code correctly the OVS packet cmd will set the skb->cb
> using the OVS_CB macro and then
> pass that packet down to the ip6erspan_tunnel_xmit function.  The
> ip6erspan_tunnel_xmit function will
> then use the IPCB macro to access the skb->cb area to set the flag to
> zero.  That works fine for packets
> generated by the system that have used the IPCB macro accessor to set the
> skb->cb data.  However, it
> is catastrophic when an skb was prepared with the OVS_CB macro because, as
> you might imagine, it
> is not pointing to an inet_skb_parm structure but an ovs_skb_cb structure
> instead.
>
> on thing I don't understand is that:
when packet/skb is sent to ip6erspan_tunnel_xmit, it is already out of OVS
code.
ip6erspan_tunnel_xmit clears the inner skb->cb, encap skb with outer
erspan+gre header,
then lookup the next netdev using outer ip address. The skb again arrives
at another netdev.
Until this point, there is no OVS code involves and so OVS_CB macro
shouldn't cause issue.

Yifeng, can you share the crash dump?
-- William

This code was *never* correct.  Somehow or other it didn't cause a problem
> until 4.15.  We'll have to
> figure out some way to check if the packet is ours or is system generated
> to know if we should be using
> the IPCB accessor.
>
> In instances of packets generated by the system or any other entity
> transmitting ipv6 packets over the
> tunnel we'll need to keep the code that clears the flags.
>
> Thanks,
>
> - Greg
>
>
> On Thu, Aug 9, 2018 at 11:17 AM, Gregory Rose 
> wrote:
>
>> On 8/9/2018 11:03 AM, Yifeng Sun wrote:
>>
>> There is a difference regarding how skb_tunnel_info is stored in skb
>> between ovs and upstream kernel. In ovs's compatible gre module,
>> skb_tunnel_info is stored in skb->cb while in upstream kernel, it is
>> referenced by skb->_skb_refdst.
>>
>> The upstream netdev code should be okay.
>>
>> To fix this issue, my guess is that either we comply to the kernel's way
>> by using skb->_skb_refdst to store skb_tunnel_info, or we don't use
>> IPCB at all.
>>
>>
>> Ah, I see...
>>
>> We must comply with the kernel method for any given kernel.  We can't be
>> sure that we'll only handle
>> or own packets.
>>
>> Can't this be fixed by a compatibility layer #define in acinclude.m4 so
>> that kernels that store it in
>> skb->cb vs. kernels that store it in skb->__skb_refdst will both work?
>>
>> Thanks,
>>
>> - Greg
>>
>>
>>
>> Thanks,
>> Yifeng
>>
>>
>>
>> On Thu, Aug 9, 2018 at 10:28 AM, Gregory Rose 
>> wrote:
>>
>>>
>>> On 8/8/2018 6:50 PM, William Tu wrote:
>>>
 thanks for the fix.

 On Wed, Aug 8, 2018 at 11:32 AM, Yifeng Sun 
 wrote:

 In compatable gre module, skb->cb is used as ovs_gso_cb.
> This bug clears the 16-23 bit in the address of ovs_gso_cb.tun_dst.
>
> can you explain more about ovs_gso_cb?

 Signed-off-by: Yifeng Sun 
> ---
>   datapath/linux/compat/ip6_gre.c | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/datapath/linux/compat/ip6_gre.c
> b/datapath/linux/compat/ip6_
> gre.c
> index 54a76ab..16c1f72 100644
> --- a/datapath/linux/compat/ip6_gre.c
> +++ b/datapath/linux/compat/ip6_gre.c
> @@ -1146,7 +1146,6 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct
> sk_buff *skb,
>  goto tx_err;
>
>  t->parms.o_flags &= ~TUNNEL_KEY;
> -   IPCB(skb)->flags = 0;
>
> The upstream linux kernel has the above code.
 Do we need to fix the upstream kernel then backport?

>>>
>>> That would be the normal work flow.
>>>
>>> Yifeng,
>>>
>>> Can you please post a patch with this fix to netdev?  Taking William's
>>> comments into account as well.
>>>
>>> Good catch and thanks for the fix!
>>>
>>> - Greg
>>>
>>>
 Thanks,
 William
 ___
 dev mailing list
 d...@openvswitch.org
 https://mail.openvswitch.org/mailman/listinfo/ovs-dev

>>>
>>>
>>
>>
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/8] ip6_gre: Fix a bug that clears address bits

[Gregory Rose]

On 8/9/2018 11:36 AM, Yifeng Sun wrote:

Yes, I agree. It should be the way to go.

Thanks,
Yifeng



If I'm reading the code correctly the OVS packet cmd will set the 
skb->cb using the OVS_CB macro and then
pass that packet down to the ip6erspan_tunnel_xmit function.  The 
ip6erspan_tunnel_xmit function will
then use the IPCB macro to access the skb->cb area to set the flag to 
zero.  That works fine for packets
generated by the system that have used the IPCB macro accessor to set 
the skb->cb data.  However, it
is catastrophic when an skb was prepared with the OVS_CB macro because, 
as you might imagine, it
is not pointing to an inet_skb_parm structure but an ovs_skb_cb 
structure instead.


This code was *never* correct.  Somehow or other it didn't cause a 
problem until 4.15.  We'll have to
figure out some way to check if the packet is ours or is system 
generated to know if we should be using

the IPCB accessor.

In instances of packets generated by the system or any other entity 
transmitting ipv6 packets over the

tunnel we'll need to keep the code that clears the flags.

Thanks,

- Greg

On Thu, Aug 9, 2018 at 11:17 AM, Gregory Rose > wrote:


On 8/9/2018 11:03 AM, Yifeng Sun wrote:

There is a difference regarding how skb_tunnel_info is stored in skb
between ovs and upstream kernel. In ovs's compatible gre module,
skb_tunnel_info is stored in skb->cb while in upstream kernel, it is
referenced by skb->_skb_refdst.

The upstream netdev code should be okay.

To fix this issue, my guess is that either we comply to the
kernel's way
by using skb->_skb_refdst to store skb_tunnel_info, or we don't use
IPCB at all.


Ah, I see...

We must comply with the kernel method for any given kernel.  We
can't be sure that we'll only handle
or own packets.

Can't this be fixed by a compatibility layer #define in
acinclude.m4 so that kernels that store it in
skb->cb vs. kernels that store it in skb->__skb_refdst will both work?

Thanks,

- Greg




Thanks,
Yifeng



On Thu, Aug 9, 2018 at 10:28 AM, Gregory Rose
mailto:gvrose8...@gmail.com>> wrote:


On 8/8/2018 6:50 PM, William Tu wrote:

thanks for the fix.

On Wed, Aug 8, 2018 at 11:32 AM, Yifeng Sun
mailto:pkusunyif...@gmail.com>>
wrote:

In compatable gre module, skb->cb is used as ovs_gso_cb.
This bug clears the 16-23 bit in the address of
ovs_gso_cb.tun_dst.

can you explain more about ovs_gso_cb?

Signed-off-by: Yifeng Sun mailto:pkusunyif...@gmail.com>>
---
  datapath/linux/compat/ip6_gre.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/datapath/linux/compat/ip6_gre.c
b/datapath/linux/compat/ip6_
gre.c
index 54a76ab..16c1f72 100644
--- a/datapath/linux/compat/ip6_gre.c
+++ b/datapath/linux/compat/ip6_gre.c
@@ -1146,7 +1146,6 @@ static netdev_tx_t
ip6erspan_tunnel_xmit(struct
sk_buff *skb,
                 goto tx_err;

         t->parms.o_flags &= ~TUNNEL_KEY;
-       IPCB(skb)->flags = 0;

The upstream linux kernel has the above code.
Do we need to fix the upstream kernel then backport?


That would be the normal work flow.

Yifeng,

Can you please post a patch with this fix to netdev?  Taking
William's comments into account as well.

Good catch and thanks for the fix!

- Greg


Thanks,
William
___
dev mailing list
d...@openvswitch.org 
https://mail.openvswitch.org/mailman/listinfo/ovs-dev









___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/8] ip6_gre: Fix a bug that clears address bits

[Yifeng Sun]

Yes, I agree. It should be the way to go.

Thanks,
Yifeng

On Thu, Aug 9, 2018 at 11:17 AM, Gregory Rose  wrote:

> On 8/9/2018 11:03 AM, Yifeng Sun wrote:
>
> There is a difference regarding how skb_tunnel_info is stored in skb
> between ovs and upstream kernel. In ovs's compatible gre module,
> skb_tunnel_info is stored in skb->cb while in upstream kernel, it is
> referenced by skb->_skb_refdst.
>
> The upstream netdev code should be okay.
>
> To fix this issue, my guess is that either we comply to the kernel's way
> by using skb->_skb_refdst to store skb_tunnel_info, or we don't use
> IPCB at all.
>
>
> Ah, I see...
>
> We must comply with the kernel method for any given kernel.  We can't be
> sure that we'll only handle
> or own packets.
>
> Can't this be fixed by a compatibility layer #define in acinclude.m4 so
> that kernels that store it in
> skb->cb vs. kernels that store it in skb->__skb_refdst will both work?
>
> Thanks,
>
> - Greg
>
>
>
> Thanks,
> Yifeng
>
>
>
> On Thu, Aug 9, 2018 at 10:28 AM, Gregory Rose 
> wrote:
>
>>
>> On 8/8/2018 6:50 PM, William Tu wrote:
>>
>>> thanks for the fix.
>>>
>>> On Wed, Aug 8, 2018 at 11:32 AM, Yifeng Sun 
>>> wrote:
>>>
>>> In compatable gre module, skb->cb is used as ovs_gso_cb.
 This bug clears the 16-23 bit in the address of ovs_gso_cb.tun_dst.

 can you explain more about ovs_gso_cb?
>>>
>>> Signed-off-by: Yifeng Sun 
 ---
   datapath/linux/compat/ip6_gre.c | 1 -
   1 file changed, 1 deletion(-)

 diff --git a/datapath/linux/compat/ip6_gre.c
 b/datapath/linux/compat/ip6_
 gre.c
 index 54a76ab..16c1f72 100644
 --- a/datapath/linux/compat/ip6_gre.c
 +++ b/datapath/linux/compat/ip6_gre.c
 @@ -1146,7 +1146,6 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct
 sk_buff *skb,
  goto tx_err;

  t->parms.o_flags &= ~TUNNEL_KEY;
 -   IPCB(skb)->flags = 0;

 The upstream linux kernel has the above code.
>>> Do we need to fix the upstream kernel then backport?
>>>
>>
>> That would be the normal work flow.
>>
>> Yifeng,
>>
>> Can you please post a patch with this fix to netdev?  Taking William's
>> comments into account as well.
>>
>> Good catch and thanks for the fix!
>>
>> - Greg
>>
>>
>>> Thanks,
>>> William
>>> ___
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>
>>
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/8] ip6_gre: Fix a bug that clears address bits

[Gregory Rose]

On 8/9/2018 11:03 AM, Yifeng Sun wrote:

There is a difference regarding how skb_tunnel_info is stored in skb
between ovs and upstream kernel. In ovs's compatible gre module,
skb_tunnel_info is stored in skb->cb while in upstream kernel, it is
referenced by skb->_skb_refdst.

The upstream netdev code should be okay.

To fix this issue, my guess is that either we comply to the kernel's way
by using skb->_skb_refdst to store skb_tunnel_info, or we don't use
IPCB at all.


Ah, I see...

We must comply with the kernel method for any given kernel.  We can't be 
sure that we'll only handle

or own packets.

Can't this be fixed by a compatibility layer #define in acinclude.m4 so 
that kernels that store it in

skb->cb vs. kernels that store it in skb->__skb_refdst will both work?

Thanks,

- Greg



Thanks,
Yifeng



On Thu, Aug 9, 2018 at 10:28 AM, Gregory Rose > wrote:



On 8/8/2018 6:50 PM, William Tu wrote:

thanks for the fix.

On Wed, Aug 8, 2018 at 11:32 AM, Yifeng Sun
mailto:pkusunyif...@gmail.com>> wrote:

In compatable gre module, skb->cb is used as ovs_gso_cb.
This bug clears the 16-23 bit in the address of
ovs_gso_cb.tun_dst.

can you explain more about ovs_gso_cb?

Signed-off-by: Yifeng Sun mailto:pkusunyif...@gmail.com>>
---
  datapath/linux/compat/ip6_gre.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/datapath/linux/compat/ip6_gre.c
b/datapath/linux/compat/ip6_
gre.c
index 54a76ab..16c1f72 100644
--- a/datapath/linux/compat/ip6_gre.c
+++ b/datapath/linux/compat/ip6_gre.c
@@ -1146,7 +1146,6 @@ static netdev_tx_t
ip6erspan_tunnel_xmit(struct
sk_buff *skb,
                 goto tx_err;

         t->parms.o_flags &= ~TUNNEL_KEY;
-       IPCB(skb)->flags = 0;

The upstream linux kernel has the above code.
Do we need to fix the upstream kernel then backport?


That would be the normal work flow.

Yifeng,

Can you please post a patch with this fix to netdev?  Taking
William's comments into account as well.

Good catch and thanks for the fix!

- Greg


Thanks,
William
___
dev mailing list
d...@openvswitch.org 
https://mail.openvswitch.org/mailman/listinfo/ovs-dev






___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/8] ip6_gre: Fix a bug that clears address bits

[Yifeng Sun]

There is a difference regarding how skb_tunnel_info is stored in skb
between ovs and upstream kernel. In ovs's compatible gre module,
skb_tunnel_info is stored in skb->cb while in upstream kernel, it is
referenced by skb->_skb_refdst.

The upstream netdev code should be okay.

To fix this issue, my guess is that either we comply to the kernel's way
by using skb->_skb_refdst to store skb_tunnel_info, or we don't use
IPCB at all.

Thanks,
Yifeng



On Thu, Aug 9, 2018 at 10:28 AM, Gregory Rose  wrote:

>
> On 8/8/2018 6:50 PM, William Tu wrote:
>
>> thanks for the fix.
>>
>> On Wed, Aug 8, 2018 at 11:32 AM, Yifeng Sun 
>> wrote:
>>
>> In compatable gre module, skb->cb is used as ovs_gso_cb.
>>> This bug clears the 16-23 bit in the address of ovs_gso_cb.tun_dst.
>>>
>>> can you explain more about ovs_gso_cb?
>>
>> Signed-off-by: Yifeng Sun 
>>> ---
>>>   datapath/linux/compat/ip6_gre.c | 1 -
>>>   1 file changed, 1 deletion(-)
>>>
>>> diff --git a/datapath/linux/compat/ip6_gre.c
>>> b/datapath/linux/compat/ip6_
>>> gre.c
>>> index 54a76ab..16c1f72 100644
>>> --- a/datapath/linux/compat/ip6_gre.c
>>> +++ b/datapath/linux/compat/ip6_gre.c
>>> @@ -1146,7 +1146,6 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct
>>> sk_buff *skb,
>>>  goto tx_err;
>>>
>>>  t->parms.o_flags &= ~TUNNEL_KEY;
>>> -   IPCB(skb)->flags = 0;
>>>
>>> The upstream linux kernel has the above code.
>> Do we need to fix the upstream kernel then backport?
>>
>
> That would be the normal work flow.
>
> Yifeng,
>
> Can you please post a patch with this fix to netdev?  Taking William's
> comments into account as well.
>
> Good catch and thanks for the fix!
>
> - Greg
>
>
>> Thanks,
>> William
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/8] ip6_gre: Fix a bug that clears address bits

[Gregory Rose]

On 8/8/2018 6:50 PM, William Tu wrote:

thanks for the fix.

On Wed, Aug 8, 2018 at 11:32 AM, Yifeng Sun  wrote:


In compatable gre module, skb->cb is used as ovs_gso_cb.
This bug clears the 16-23 bit in the address of ovs_gso_cb.tun_dst.


can you explain more about ovs_gso_cb?


Signed-off-by: Yifeng Sun 
---
  datapath/linux/compat/ip6_gre.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/datapath/linux/compat/ip6_gre.c b/datapath/linux/compat/ip6_
gre.c
index 54a76ab..16c1f72 100644
--- a/datapath/linux/compat/ip6_gre.c
+++ b/datapath/linux/compat/ip6_gre.c
@@ -1146,7 +1146,6 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct
sk_buff *skb,
 goto tx_err;

 t->parms.o_flags &= ~TUNNEL_KEY;
-   IPCB(skb)->flags = 0;


The upstream linux kernel has the above code.
Do we need to fix the upstream kernel then backport?


That would be the normal work flow.

Yifeng,

Can you please post a patch with this fix to netdev?  Taking William's 
comments into account as well.


Good catch and thanks for the fix!

- Greg



Thanks,
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/8] ip6_gre: Fix a bug that clears address bits

[William Tu]

thanks for the fix.

On Wed, Aug 8, 2018 at 11:32 AM, Yifeng Sun  wrote:

> In compatable gre module, skb->cb is used as ovs_gso_cb.
> This bug clears the 16-23 bit in the address of ovs_gso_cb.tun_dst.
>

can you explain more about ovs_gso_cb?

>
> Signed-off-by: Yifeng Sun 
> ---
>  datapath/linux/compat/ip6_gre.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/datapath/linux/compat/ip6_gre.c b/datapath/linux/compat/ip6_
> gre.c
> index 54a76ab..16c1f72 100644
> --- a/datapath/linux/compat/ip6_gre.c
> +++ b/datapath/linux/compat/ip6_gre.c
> @@ -1146,7 +1146,6 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct
> sk_buff *skb,
> goto tx_err;
>
> t->parms.o_flags &= ~TUNNEL_KEY;
> -   IPCB(skb)->flags = 0;
>

The upstream linux kernel has the above code.
Do we need to fix the upstream kernel then backport?

Thanks,
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev