On 10/31/2018 5:40 AM, Jaime Caamaño Ruiz wrote:
Greg, I submitted this patch [1], let me know if anything looks bad.

[1] https://mail.openvswitch.org/pipermail/ovs-dev/2018-October/353410.html

I'll have a look and comment there.

Thanks!


Thanks
Jaime.


-----Original Message-----
From: Jaime Caamaño Ruiz <jcaam...@suse.de>
Reply-to: jcaam...@suse.com
To: Gregory Rose <gvrose8...@gmail.com>, ovs-discuss@openvswitch.org
Subject: Re: [ovs-discuss] Problems executing decap(eth)+encap(eth)
actions
Date: Wed, 31 Oct 2018 12:07:59 +0100

Let me give it a try.

Aside for the fix on master, who takes care of mapping the fix to
bugfix releases?

BR
Jaime.

-----Original Message-----
From: Gregory Rose <gvrose8...@gmail.com>
To: ovs-discuss@openvswitch.org, jcaam...@suse.de
Subject: Re: [ovs-discuss] Problems executing decap(eth)+encap(eth)
actions
Date: Tue, 30 Oct 2018 14:42:15 -0700

On 10/29/2018 3:38 AM, Jaime Caamaño Ruiz wrote:
Hey Greg. Thanks for helping out. I did build OVS with the fix and it
got my problem sorted without causing any additional ones on my
environment. Let me know if I can help with anything else.

BR
Jaime.
Jaime,

you seem to have identified a bug!

Using printks with a simple rule to just decap and then encap an
Ethernet header we see this with the code
as it is right now:

[13568.973807] __ovs_nla_copy_actions:3007 <- decap
[13568.973812] __ovs_nla_copy_actions:3012 <- decap succeeds but sets
mac_proto = MAC_PROTO_ETHERNET
[13568.973815] __ovs_nla_copy_actions:2999 <- encap
[13568.973818] openvswitch: netlink: Flow actions may not be safe on
all
matching packets. <- returns -EINVAL

Note that the decap happens at lines 3007-3012 and is successful.
However, the very next encap action
starting at line 2999 does not finish and returns -EINVAL so a printk
at
line 3002 does not execute.
If I change the code as you suggested the flow of decap/encap works
without complaint and without
returning -EINVAL:

[13838.435051] __ovs_nla_copy_actions:3007 <- decap
[13838.435054] __ovs_nla_copy_actions:3012 <-decap succeeds and sets
mac_proto = MAC_PROTO_NONE
[13838.435055] __ovs_nla_copy_actions:2999 <- encap
[13838.435056] __ovs_nla_copy_actions:3002 <- encap succeeds and sets
mac_proto = MAC_PROTO_ETHERNET

Thank you for finding this bug.  Do you wish to send the patch to fix
it
or would you prefer me to do it?

Regards,

- Greg


-----Original Message-----
From: Gregory Rose <gvrose8...@gmail.com>
To: ovs-discuss@openvswitch.org, jcaam...@suse.de
Subject: Re: [ovs-discuss] Problems executing decap(eth)+encap(eth)
actions
Date: Fri, 26 Oct 2018 15:42:51 -0700

On 10/19/2018 1:39 AM, Jaime Caamaño Ruiz wrote:
Hello

When using nsh encapsulation, it's useful to normalize your
pipeline
to
packet_type=nsh, poping an ethernet header on input if necessary
and
pushing an ethernet header again if required before output.

But it seems to be problematic:

---
2018-10-18T13:10:59.196Z|00010|dpif(handler3)|WARN|system@ovs-syste
m:
execute
pop_eth,push_eth(src=fe:16:3e:c1:9e:87,dst=fa:16:3e:c1:9e:87),5
failed (Invalid argument) on packet
vlan_tci=0x0000,dl_src=fa:16:3e:c2:e6:68,dl_dst=fe:16:3e:c2:e6:68,d
l_
ty
pe=0x894f,nsh_flags=0,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x1a
,n
sh
_si=254,nsh_c1=0xc0a82a01,nsh_c2=0x3,nsh_c3=0x0,nsh_c4=0x90000100,n
w_
pr
oto=0,nw_tos=0,nw_ecn=0,nw_ttl=0
    with metadata
skb_priority(0),tunnel(tun_id=0x0,src=192.168.42.1,dst=192.168.42.3
,t
tl
=64,tp_src=47656,tp_dst=4789,flags(key)),skb_mark(0),in_port(4) mtu
0
---

Looking at the code datapath/flow_netlink.c @
__ovs_nla_copy_actions:

                   case OVS_ACTION_ATTR_PUSH_ETH:
                           /* Disallow pushing an Ethernet header if
one
                            * is already present */
                           if (mac_proto != MAC_PROTO_NONE)
                                   return -EINVAL;
                           mac_proto = MAC_PROTO_NONE;
                           break;

                   case OVS_ACTION_ATTR_POP_ETH:
                           if (mac_proto != MAC_PROTO_ETHERNET)
                                   return -EINVAL;
                           if (vlan_tci & htons(VLAN_TAG_PRESENT))
                                   return -EINVAL;
                           mac_proto = MAC_PROTO_ETHERNET;
                           break;


Isn't the mac_proto set inverted here, should'nt it look like this?


                   case OVS_ACTION_ATTR_PUSH_ETH:
                           /* Disallow pushing an Ethernet header if
one
                            * is already present */
                           if (mac_proto != MAC_PROTO_NONE)
                                   return -EINVAL;
                           mac_proto = MAC_PROTO_ETHERNET;
                           break;

                   case OVS_ACTION_ATTR_POP_ETH:
                           if (mac_proto != MAC_PROTO_ETHERNET)
                                   return -EINVAL;
                           if (vlan_tci & htons(VLAN_TAG_PRESENT))
                                   return -EINVAL;
                           mac_proto = MAC_PROTO_NONE;
                           break;
Jaime,

I am looking into this and at first sight this does look inverted but
we
have no other reported bugs
in this area so I want to be careful that we don't break anything
else
while fixing this.  Have you tried
building OVS with this code change and been able to verify that this
change works?

I'll be working on setting up a test bed to check this - hopefully by
early next week I can report back so
stay tuned please.

Thanks,

- Greg

BR
Jaime.
_______________________________________________
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss


_______________________________________________
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss

Reply via email to