[ovs-discuss] Problems executing decap(eth)+encap(eth) actions

[Gregory Rose]

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 <gvrose8192 at gmail.com>
> To: ovs-discuss at openvswitch.org, jcaamano at 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 at ovs-system:
>> 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,dl_
>> 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,nw_
>> 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
>> discuss at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
>