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
Thanks Jaime. -----Original Message----- From: Jaime Caamaño Ruiz <[email protected]> Reply-to: [email protected] To: Gregory Rose <[email protected]>, [email protected] 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 <[email protected]> To: [email protected], [email protected] 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 <[email protected]> > To: [email protected], [email protected] > 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 > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-discuss _______________________________________________ discuss mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
