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

Message ID 1533753171-11730-1-git-send-email-pkusunyifeng@gmail.com
State New
Headers show
Series
  • [ovs-dev,1/8] ip6_gre: Fix a bug that clears address bits
Related show

Commit Message

Yifeng Sun Aug. 8, 2018, 6:32 p.m.
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 <pkusunyifeng@gmail.com>
---
 datapath/linux/compat/ip6_gre.c | 1 -
 1 file changed, 1 deletion(-)

Comments

William Tu Aug. 9, 2018, 1:50 a.m. | #1
thanks for the fix.

On Wed, Aug 8, 2018 at 11:32 AM, Yifeng Sun <pkusunyifeng@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 <pkusunyifeng@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?

Thanks,
William
Gregory Rose Aug. 9, 2018, 5:28 p.m. | #2
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 <pkusunyifeng@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 <pkusunyifeng@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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Yifeng Sun Aug. 9, 2018, 6:03 p.m. | #3
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 <gvrose8192@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 <pkusunyifeng@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 <pkusunyifeng@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
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
>
Gregory Rose Aug. 9, 2018, 6:17 p.m. | #4
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 <gvrose8192@gmail.com 
> <mailto:gvrose8192@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
>         <pkusunyifeng@gmail.com <mailto:pkusunyifeng@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 <pkusunyifeng@gmail.com
>             <mailto:pkusunyifeng@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
>         dev@openvswitch.org <mailto:dev@openvswitch.org>
>         https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>         <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
>
>
>
Yifeng Sun Aug. 9, 2018, 6:36 p.m. | #5
Yes, I agree. It should be the way to go.

Thanks,
Yifeng

On Thu, Aug 9, 2018 at 11:17 AM, Gregory Rose <gvrose8192@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 <gvrose8192@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 <pkusunyifeng@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 <pkusunyifeng@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
>>> dev@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>
>>
>
>
Gregory Rose Aug. 9, 2018, 9:37 p.m. | #6
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 <gvrose8192@gmail.com 
> <mailto:gvrose8192@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
>>     <gvrose8192@gmail.com <mailto:gvrose8192@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
>>             <pkusunyifeng@gmail.com <mailto:pkusunyifeng@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 <pkusunyifeng@gmail.com
>>                 <mailto:pkusunyifeng@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
>>             dev@openvswitch.org <mailto:dev@openvswitch.org>
>>             https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>             <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
>>
>>
>>
>
>
William Tu Aug. 9, 2018, 10:33 p.m. | #7
On Thu, Aug 9, 2018 at 2:37 PM, Gregory Rose <gvrose8192@gmail.com> 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 <gvrose8192@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 <gvrose8192@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 <pkusunyifeng@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 <pkusunyifeng@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
>>>> dev@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>
>>>
>>>
>>
>>
>
>
Gregory Rose Aug. 9, 2018, 10:44 p.m. | #8
On 8/9/2018 3:33 PM, William Tu wrote:
>
>
> On Thu, Aug 9, 2018 at 2:37 PM, Gregory Rose <gvrose8192@gmail.com 
> <mailto:gvrose8192@gmail.com>> 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
>>     <gvrose8192@gmail.com <mailto:gvrose8192@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
>>>         <gvrose8192@gmail.com <mailto:gvrose8192@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
>>>                 <pkusunyifeng@gmail.com
>>>                 <mailto:pkusunyifeng@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
>>>                     <pkusunyifeng@gmail.com
>>>                     <mailto:pkusunyifeng@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
>>>                 dev@openvswitch.org <mailto:dev@openvswitch.org>
>>>                 https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>                 <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
>>>
>>>
>>>
>>
>>
>
>
Gregory Rose Aug. 9, 2018, 10:59 p.m. | #9
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 <pkusunyifeng@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;
>   
>   	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
Yifeng Sun Aug. 9, 2018, 11:14 p.m. | #10
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: 0000 [#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:ffffb36d008177c0 EFLAGS: 00010286
[  645.840562] RAX: 00000000ffffffea RBX: ffff9be571e84000 RCX:
0000000000000040
[  645.841126] RDX: 000000000000003e RSI: ffff000071cf7e00 RDI:
ffff9be571cf7600
[  645.841697] RBP: ffffb36d00817880 R08: 0000000000000018 R09:
0000000000000000
[  645.842268] R10: ffffb36d00817a90 R11: 00000000fffffffe R12:
ffff9be5610e4400
[  645.842838] R13: 0000000000000000 R14: 0000000000000001 R15:
0000000000000000
[  645.843422] FS:  00007eff6e146700(0000) GS:ffff9be5bfc00000(0000)
knlGS:0000000000000000
[  645.844015] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  645.844607] CR2: 00007febc8f859d0 CR3: 000000005de8a002 CR4:
00000000001606f0
[  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:00007eff6e0e81a0 EFLAGS: 00000293 ORIG_RAX:
000000000000002e
[  645.861226] RAX: ffffffffffffffda RBX: 0000000000000002 RCX:
00007eff6ec36a6d
[  645.861622] RDX: 0000000000000000 RSI: 00007eff6e0e8200 RDI:
0000000000000011
[  645.862009] RBP: 00007eff6e0e8fe0 R08: 0000000000000000 R09:
00007eff6e0e9a60
[  645.862396] R10: 0000000000a5d340 R11: 0000000000000293 R12:
00000000009f7690
[  645.862775] R13: 00007eff6e0e8200 R14: 0000000000000002 R15:
00007eff6e0e86a0
[  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:
ffffb36d008177c0
[  645.865043] ---[ end trace 25bab82318677b6f ]---
[  645.865452] Kernel panic - not syncing: Fatal exception in interrupt
[  645.865923] Kernel Offset: 0x3d000000 from 0xffffffff81000000
(relocation range: 0xffffffff80000000-0xffffffffbfffffff)
[  645.866752] ---[ end Kernel panic - not syncing: Fatal exception in
interrupt


On Thu, Aug 9, 2018 at 3:59 PM, Gregory Rose <gvrose8192@gmail.com> 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_gso_cb.tun_dst.
>>
>> Signed-off-by: Yifeng Sun <pkusunyifeng@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;
>>         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
>

Patch

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 ||