| Message ID | 1608075680-99704-1-git-send-email-yihung.wei@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] netdev-natvie-tnl: Fix geneve tunnel flag | expand |
Thanks for working on the patch. I have some discussions with Yi-Hung and Toms. Add comments inline. On Tue, Dec 15, 2020 at 3:41 PM Yi-Hung Wei <yihung.wei@gmail.com> wrote: > > On userspace datapath, geneve option flag FLOW_TNL_F_UDPIF should only > be set when the geneve option is available. However, currently, we always > set FLOW_TNL_F_UDPIF in the metadata flag, and that may result in megaflow > revalidation issue when the geneve option length is zero due to the datapath > flow key mis-match. That is the original flow key always has FLOW_TNL_F_UDPIF > set, while the regenerated flow key during revalidation process only has > FLOW_TNL_F_UDPIF set if geneve option length is zero. For reference, check > tun_metadata_to_geneve_nlattr() to see how the flow key of geneve > tunnel metadatafor are generated from netlink attributes. > The issue shows up when using Geneve without geneve TLV metadata. That's why it does not show up in OVN use case or VMware's internal case. > The following are the reproducible steps reported by Antonin. > After step 6, OvS reports a warning about failing to update the datapath > flow. > > 2020-12-15T01:56:21.324Z|00007|dpif(revalidator4)|WARN|netdev@ovs-netdev: > failed to put[modify] (No such file or directory) > ufid:d252fe62-5b2a-44e6-846a-190328190e09 skb_priority(0/0), > tunnel(tun_id=0x0,src=192.168.77.1,dst=192.168.77.2,ttl=64/0,tp_src=57391/0, > tp_dst=6081/0,flags(-df-csum+key)),skb_mark(0/0),ct_state(0x21/0x21), > ct_zone(0xfff0/0),ct_mark(0/0),ct_label(0/0),ct_tuple4(src=10.10.0.1/0.0.0.0, > dst=10.10.0.2/0.0.0.0,proto=1/0,tp_src=8/0,tp_dst=0/0),recirc_id(0xa), > dp_hash(0/0),in_port(5),packet_type(ns=0,id=0), > eth(src=1e:9c:f0:fb:18:9c/00:00:00:00:00:00,dst=ee:9f:60:8a:c0:a5/00:00:00:00:00:00), > eth_type(0x0800),ipv4(src=10.10.0.1/0.0.0.0,dst=10.10.0.2,proto=1/0,tos=0/0, > ttl=64/0,frag=no),icmp(type=8/0,code=0/0), actions:drop So the reason revalidator has error is due to revalidator creates a flow without setting the FLOW_TNL_F_UDPIF (due to length zero). While the existing megaflow cache has FLOW_TNL_F_UDPIF set? > > Setup: > 2 Nodes on the same subnet (192.168.77.101/24 - 192.168.77.102/24) > > Step 1 – Creating bridges (run on each Node): > > iface="enp0s8" > > hwaddr=$(ip link show $iface | grep link/ether | awk '{print $2}') > inet=$(ip addr show $iface | grep "inet " | awk '{ print $2 }') > > ovs-vsctl add-br br-phy \ > -- set Bridge br-phy datapath_type=netdev \ > -- br-set-external-id br-phy bridge-id br-phy \ > -- set bridge br-phy fail-mode=standalone \ > other_config:hwaddr="$hwaddr" > ovs-vsctl --timeout 10 add-port br-phy "$iface" > ip addr add "$inet" dev br-phy > ip link set br-phy up > ip addr flush dev enp0s8 2>/dev/null > ip link set enp0s8 up > iptables -F > > ovs-vsctl add-br br-int \ > -- set Bridge br-int datapath_type=netdev \ > -- set bridge br-phy fail-mode=standalone > > Step 2 – Creating netns to for overlay endpoints: > > On first Node (192.168.77.101): > ip netns add ns0 > ip link add veth0 type veth peer name veth1 > ip link set veth0 netns ns0 > ovs-vsctl add-port br-int veth1 > ip link set dev veth1 up > ip netns exec ns0 ip addr add 10.10.0.1/24 dev veth0 > ip netns exec ns0 ip link set dev veth0 up > > On second Node (192.168.77.102): > ip netns add ns0 > ip link add veth0 type veth peer name veth1 > ip link set veth0 netns ns0 > ovs-vsctl add-port br-int veth1 > ip link set dev veth1 up > ip netns exec ns0 ip addr add 10.10.0.2/24 dev veth0 > ip netns exec ns0 ip link set dev veth0 up > > Step 3 – Create tunnel (run on each Node): > > ovs-vsctl add-port br-int tun0 -- set interface tun0 type=geneve \ > ofport_request=100 options:remote_ip=flow options:key=flow > > Step 4 – Create the following flows: > > On first Node (192.168.77.101): > root@ovs-test-node-1:/home/vagrant# ovs-ofctl dump-flows br-int --no-stats > priority=100,ip actions=resubmit(,10) > priority=0 actions=NORMAL > priority=50 actions=resubmit(,40) > table=10, priority=100,ip actions=ct(table=20,zone=65520) > table=20, priority=200,ct_state=-new+trk,ip actions=resubmit(,30) > table=20, priority=100,ip,nw_dst=10.10.0.2 actions=resubmit(,30) > table=20, priority=0,ip actions=drop > table=30, priority=100,ip actions=ct(commit,table=40,zone=65520) > table=40, priority=100,in_port=veth1 actions=load:0xc0a84d66->NXM_NX_TUN_IPV4_DST[],output:tun0 > table=40, priority=100,in_port=tun0 actions=output:veth1 > table=40, priority=0 actions=drop > > On second Node (192.168.77.102): > root@ovs-test-node-2:/home/vagrant# ovs-ofctl dump-flows br-int --no-stats > priority=100,ip actions=resubmit(,10) > priority=0 actions=NORMAL > priority=50 actions=resubmit(,40) > table=10, priority=100,ip actions=ct(table=20,zone=65520) > table=20, priority=200,ct_state=-new+trk,ip actions=resubmit(,30) > table=20, priority=100,ip,nw_dst=10.10.0.2 actions=resubmit(,30) > table=20, priority=0,ip actions=drop > table=30, priority=100,ip actions=ct(commit,table=40,zone=65520) > table=40, priority=100,in_port=veth1 actions=load:0xc0a84d65->NXM_NX_TUN_IPV4_DST[],output:tun0 > table=40, priority=100,in_port=tun0 actions=output:veth1 > table=40, priority=0 actions=drop > > Step 5 – Check that ping works: > From the first Node: ip netns exec ns0 ping 10.10.0.2 > > Step 6 – The actual issue: > From the first Node: ip netns exec ns0 ping -c 3 10.10.0.2 > From the second Node: ovs-ofctl del-flows br-int 'table=20,ip,nw_dst=10.10.0.2' > From the first Node: ip netns exec ns0 ping -c 3 10.10.0.2 Thanks for the steps. The bug should be reproducible without using conntrack. > > Execute these instructions in order, as soon as the previous one > completes. If you follow these steps, you should see the ping in > the last step succeed. This is not expected because of the deleted > flow. It also does not happen with VXLAN. Make sense, because it's the inconsistency in FLOW_TNL_F_UDPIF field in megaflow matching. VXLAN does not use it. > Reported-by: Antonin Bas <abas@vmware.com> > Reported-at: https://github.com/vmware-tanzu/antrea/issues/897 > Fixes: 6b241d645291 ("netdev-vport: Factor-out tunnel Push-pop code into separate module.") > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> VMWare-BZ: #2623444 > --- > lib/netdev-native-tnl.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c > index b89dfdd52a86..09cff3a1fc7d 100644 > --- a/lib/netdev-native-tnl.c > +++ b/lib/netdev-native-tnl.c > @@ -1015,9 +1015,11 @@ netdev_geneve_pop_header(struct dp_packet *packet) > tnl->tun_id = htonll(ntohl(get_16aligned_be32(&gnh->vni)) >> 8); > tnl->flags |= FLOW_TNL_F_KEY; > > - memcpy(tnl->metadata.opts.gnv, gnh->options, opts_len); > - tnl->metadata.present.len = opts_len; > - tnl->flags |= FLOW_TNL_F_UDPIF; > + if (opts_len > 0) { > + memcpy(tnl->metadata.opts.gnv, gnh->options, opts_len); > + tnl->metadata.present.len = opts_len; > + tnl->flags |= FLOW_TNL_F_UDPIF; > + } > > packet->packet_type = htonl(PT_ETH); > dp_packet_reset_packet(packet, hlen); > -- LGTM. Acked-by: William Tu <u9012063@gmail.com>
On 12/16/20 12:41 AM, Yi-Hung Wei wrote: > On userspace datapath, geneve option flag FLOW_TNL_F_UDPIF should only > be set when the geneve option is available. However, currently, we always > set FLOW_TNL_F_UDPIF in the metadata flag, and that may result in megaflow > revalidation issue when the geneve option length is zero due to the datapath > flow key mis-match. That is the original flow key always has FLOW_TNL_F_UDPIF > set, while the regenerated flow key during revalidation process only has > FLOW_TNL_F_UDPIF set if geneve option length is zero. For reference, check > tun_metadata_to_geneve_nlattr() to see how the flow key of geneve > tunnel metadatafor are generated from netlink attributes. > > The following are the reproducible steps reported by Antonin. > After step 6, OvS reports a warning about failing to update the datapath > flow. > > 2020-12-15T01:56:21.324Z|00007|dpif(revalidator4)|WARN|netdev@ovs-netdev: > failed to put[modify] (No such file or directory) > ufid:d252fe62-5b2a-44e6-846a-190328190e09 skb_priority(0/0), > tunnel(tun_id=0x0,src=192.168.77.1,dst=192.168.77.2,ttl=64/0,tp_src=57391/0, > tp_dst=6081/0,flags(-df-csum+key)),skb_mark(0/0),ct_state(0x21/0x21), > ct_zone(0xfff0/0),ct_mark(0/0),ct_label(0/0),ct_tuple4(src=10.10.0.1/0.0.0.0, > dst=10.10.0.2/0.0.0.0,proto=1/0,tp_src=8/0,tp_dst=0/0),recirc_id(0xa), > dp_hash(0/0),in_port(5),packet_type(ns=0,id=0), > eth(src=1e:9c:f0:fb:18:9c/00:00:00:00:00:00,dst=ee:9f:60:8a:c0:a5/00:00:00:00:00:00), > eth_type(0x0800),ipv4(src=10.10.0.1/0.0.0.0,dst=10.10.0.2,proto=1/0,tos=0/0, > ttl=64/0,frag=no),icmp(type=8/0,code=0/0), actions:drop > > Setup: > 2 Nodes on the same subnet (192.168.77.101/24 - 192.168.77.102/24) > > Step 1 – Creating bridges (run on each Node): > > iface="enp0s8" > > hwaddr=$(ip link show $iface | grep link/ether | awk '{print $2}') > inet=$(ip addr show $iface | grep "inet " | awk '{ print $2 }') > > ovs-vsctl add-br br-phy \ > -- set Bridge br-phy datapath_type=netdev \ > -- br-set-external-id br-phy bridge-id br-phy \ > -- set bridge br-phy fail-mode=standalone \ > other_config:hwaddr="$hwaddr" > ovs-vsctl --timeout 10 add-port br-phy "$iface" > ip addr add "$inet" dev br-phy > ip link set br-phy up > ip addr flush dev enp0s8 2>/dev/null > ip link set enp0s8 up > iptables -F > > ovs-vsctl add-br br-int \ > -- set Bridge br-int datapath_type=netdev \ > -- set bridge br-phy fail-mode=standalone > > Step 2 – Creating netns to for overlay endpoints: > > On first Node (192.168.77.101): > ip netns add ns0 > ip link add veth0 type veth peer name veth1 > ip link set veth0 netns ns0 > ovs-vsctl add-port br-int veth1 > ip link set dev veth1 up > ip netns exec ns0 ip addr add 10.10.0.1/24 dev veth0 > ip netns exec ns0 ip link set dev veth0 up > > On second Node (192.168.77.102): > ip netns add ns0 > ip link add veth0 type veth peer name veth1 > ip link set veth0 netns ns0 > ovs-vsctl add-port br-int veth1 > ip link set dev veth1 up > ip netns exec ns0 ip addr add 10.10.0.2/24 dev veth0 > ip netns exec ns0 ip link set dev veth0 up > > Step 3 – Create tunnel (run on each Node): > > ovs-vsctl add-port br-int tun0 -- set interface tun0 type=geneve \ > ofport_request=100 options:remote_ip=flow options:key=flow > > Step 4 – Create the following flows: > > On first Node (192.168.77.101): > root@ovs-test-node-1:/home/vagrant# ovs-ofctl dump-flows br-int --no-stats > priority=100,ip actions=resubmit(,10) > priority=0 actions=NORMAL > priority=50 actions=resubmit(,40) > table=10, priority=100,ip actions=ct(table=20,zone=65520) > table=20, priority=200,ct_state=-new+trk,ip actions=resubmit(,30) > table=20, priority=100,ip,nw_dst=10.10.0.2 actions=resubmit(,30) > table=20, priority=0,ip actions=drop > table=30, priority=100,ip actions=ct(commit,table=40,zone=65520) > table=40, priority=100,in_port=veth1 actions=load:0xc0a84d66->NXM_NX_TUN_IPV4_DST[],output:tun0 > table=40, priority=100,in_port=tun0 actions=output:veth1 > table=40, priority=0 actions=drop > > On second Node (192.168.77.102): > root@ovs-test-node-2:/home/vagrant# ovs-ofctl dump-flows br-int --no-stats > priority=100,ip actions=resubmit(,10) > priority=0 actions=NORMAL > priority=50 actions=resubmit(,40) > table=10, priority=100,ip actions=ct(table=20,zone=65520) > table=20, priority=200,ct_state=-new+trk,ip actions=resubmit(,30) > table=20, priority=100,ip,nw_dst=10.10.0.2 actions=resubmit(,30) > table=20, priority=0,ip actions=drop > table=30, priority=100,ip actions=ct(commit,table=40,zone=65520) > table=40, priority=100,in_port=veth1 actions=load:0xc0a84d65->NXM_NX_TUN_IPV4_DST[],output:tun0 > table=40, priority=100,in_port=tun0 actions=output:veth1 > table=40, priority=0 actions=drop > > Step 5 – Check that ping works: > From the first Node: ip netns exec ns0 ping 10.10.0.2 > > Step 6 – The actual issue: > From the first Node: ip netns exec ns0 ping -c 3 10.10.0.2 > From the second Node: ovs-ofctl del-flows br-int 'table=20,ip,nw_dst=10.10.0.2' > From the first Node: ip netns exec ns0 ping -c 3 10.10.0.2 > > Execute these instructions in order, as soon as the previous one > completes. If you follow these steps, you should see the ping in > the last step succeed. This is not expected because of the deleted > flow. It also does not happen with VXLAN. > Since you already know how to reproduce the issue, could you, please, create a system test out of these steps or more simplified ones without conntrack? A unit test would be ideal, though. I didn't review the patch itself, but the issue seems not easy to analyze and very easy to re-introduce someday without regression tests. Best regards, Ilya Maximets. > Reported-by: Antonin Bas <abas@vmware.com> > Reported-at: https://github.com/vmware-tanzu/antrea/issues/897 > Fixes: 6b241d645291 ("netdev-vport: Factor-out tunnel Push-pop code into separate module.") > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> > --- > lib/netdev-native-tnl.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c > index b89dfdd52a86..09cff3a1fc7d 100644 > --- a/lib/netdev-native-tnl.c > +++ b/lib/netdev-native-tnl.c > @@ -1015,9 +1015,11 @@ netdev_geneve_pop_header(struct dp_packet *packet) > tnl->tun_id = htonll(ntohl(get_16aligned_be32(&gnh->vni)) >> 8); > tnl->flags |= FLOW_TNL_F_KEY; > > - memcpy(tnl->metadata.opts.gnv, gnh->options, opts_len); > - tnl->metadata.present.len = opts_len; > - tnl->flags |= FLOW_TNL_F_UDPIF; > + if (opts_len > 0) { > + memcpy(tnl->metadata.opts.gnv, gnh->options, opts_len); > + tnl->metadata.present.len = opts_len; > + tnl->flags |= FLOW_TNL_F_UDPIF; > + } > > packet->packet_type = htonl(PT_ETH); > dp_packet_reset_packet(packet, hlen); >
On Wed, Dec 16, 2020 at 1:24 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 12/16/20 12:41 AM, Yi-Hung Wei wrote: > > On userspace datapath, geneve option flag FLOW_TNL_F_UDPIF should only > > be set when the geneve option is available. However, currently, we always > > set FLOW_TNL_F_UDPIF in the metadata flag, and that may result in megaflow > > revalidation issue when the geneve option length is zero due to the datapath > > flow key mis-match. That is the original flow key always has FLOW_TNL_F_UDPIF > > set, while the regenerated flow key during revalidation process only has > > FLOW_TNL_F_UDPIF set if geneve option length is zero. For reference, check > > tun_metadata_to_geneve_nlattr() to see how the flow key of geneve > > tunnel metadatafor are generated from netlink attributes. > > > > The following are the reproducible steps reported by Antonin. > > After step 6, OvS reports a warning about failing to update the datapath > > flow. > > > > 2020-12-15T01:56:21.324Z|00007|dpif(revalidator4)|WARN|netdev@ovs-netdev: > > failed to put[modify] (No such file or directory) > > ufid:d252fe62-5b2a-44e6-846a-190328190e09 skb_priority(0/0), > > tunnel(tun_id=0x0,src=192.168.77.1,dst=192.168.77.2,ttl=64/0,tp_src=57391/0, > > tp_dst=6081/0,flags(-df-csum+key)),skb_mark(0/0),ct_state(0x21/0x21), > > ct_zone(0xfff0/0),ct_mark(0/0),ct_label(0/0),ct_tuple4(src=10.10.0.1/0.0.0.0, > > dst=10.10.0.2/0.0.0.0,proto=1/0,tp_src=8/0,tp_dst=0/0),recirc_id(0xa), > > dp_hash(0/0),in_port(5),packet_type(ns=0,id=0), > > eth(src=1e:9c:f0:fb:18:9c/00:00:00:00:00:00,dst=ee:9f:60:8a:c0:a5/00:00:00:00:00:00), > > eth_type(0x0800),ipv4(src=10.10.0.1/0.0.0.0,dst=10.10.0.2,proto=1/0,tos=0/0, > > ttl=64/0,frag=no),icmp(type=8/0,code=0/0), actions:drop > > > > Setup: > > 2 Nodes on the same subnet (192.168.77.101/24 - 192.168.77.102/24) > > > > Step 1 – Creating bridges (run on each Node): > > > > iface="enp0s8" > > > > hwaddr=$(ip link show $iface | grep link/ether | awk '{print $2}') > > inet=$(ip addr show $iface | grep "inet " | awk '{ print $2 }') > > > > ovs-vsctl add-br br-phy \ > > -- set Bridge br-phy datapath_type=netdev \ > > -- br-set-external-id br-phy bridge-id br-phy \ > > -- set bridge br-phy fail-mode=standalone \ > > other_config:hwaddr="$hwaddr" > > ovs-vsctl --timeout 10 add-port br-phy "$iface" > > ip addr add "$inet" dev br-phy > > ip link set br-phy up > > ip addr flush dev enp0s8 2>/dev/null > > ip link set enp0s8 up > > iptables -F > > > > ovs-vsctl add-br br-int \ > > -- set Bridge br-int datapath_type=netdev \ > > -- set bridge br-phy fail-mode=standalone > > > > Step 2 – Creating netns to for overlay endpoints: > > > > On first Node (192.168.77.101): > > ip netns add ns0 > > ip link add veth0 type veth peer name veth1 > > ip link set veth0 netns ns0 > > ovs-vsctl add-port br-int veth1 > > ip link set dev veth1 up > > ip netns exec ns0 ip addr add 10.10.0.1/24 dev veth0 > > ip netns exec ns0 ip link set dev veth0 up > > > > On second Node (192.168.77.102): > > ip netns add ns0 > > ip link add veth0 type veth peer name veth1 > > ip link set veth0 netns ns0 > > ovs-vsctl add-port br-int veth1 > > ip link set dev veth1 up > > ip netns exec ns0 ip addr add 10.10.0.2/24 dev veth0 > > ip netns exec ns0 ip link set dev veth0 up > > > > Step 3 – Create tunnel (run on each Node): > > > > ovs-vsctl add-port br-int tun0 -- set interface tun0 type=geneve \ > > ofport_request=100 options:remote_ip=flow options:key=flow > > > > Step 4 – Create the following flows: > > > > On first Node (192.168.77.101): > > root@ovs-test-node-1:/home/vagrant# ovs-ofctl dump-flows br-int --no-stats > > priority=100,ip actions=resubmit(,10) > > priority=0 actions=NORMAL > > priority=50 actions=resubmit(,40) > > table=10, priority=100,ip actions=ct(table=20,zone=65520) > > table=20, priority=200,ct_state=-new+trk,ip actions=resubmit(,30) > > table=20, priority=100,ip,nw_dst=10.10.0.2 actions=resubmit(,30) > > table=20, priority=0,ip actions=drop > > table=30, priority=100,ip actions=ct(commit,table=40,zone=65520) > > table=40, priority=100,in_port=veth1 actions=load:0xc0a84d66->NXM_NX_TUN_IPV4_DST[],output:tun0 > > table=40, priority=100,in_port=tun0 actions=output:veth1 > > table=40, priority=0 actions=drop > > > > On second Node (192.168.77.102): > > root@ovs-test-node-2:/home/vagrant# ovs-ofctl dump-flows br-int --no-stats > > priority=100,ip actions=resubmit(,10) > > priority=0 actions=NORMAL > > priority=50 actions=resubmit(,40) > > table=10, priority=100,ip actions=ct(table=20,zone=65520) > > table=20, priority=200,ct_state=-new+trk,ip actions=resubmit(,30) > > table=20, priority=100,ip,nw_dst=10.10.0.2 actions=resubmit(,30) > > table=20, priority=0,ip actions=drop > > table=30, priority=100,ip actions=ct(commit,table=40,zone=65520) > > table=40, priority=100,in_port=veth1 actions=load:0xc0a84d65->NXM_NX_TUN_IPV4_DST[],output:tun0 > > table=40, priority=100,in_port=tun0 actions=output:veth1 > > table=40, priority=0 actions=drop > > > > Step 5 – Check that ping works: > > From the first Node: ip netns exec ns0 ping 10.10.0.2 > > > > Step 6 – The actual issue: > > From the first Node: ip netns exec ns0 ping -c 3 10.10.0.2 > > From the second Node: ovs-ofctl del-flows br-int 'table=20,ip,nw_dst=10.10.0.2' > > From the first Node: ip netns exec ns0 ping -c 3 10.10.0.2 > > > > Execute these instructions in order, as soon as the previous one > > completes. If you follow these steps, you should see the ping in > > the last step succeed. This is not expected because of the deleted > > flow. It also does not happen with VXLAN. > > > > Since you already know how to reproduce the issue, could you, please, create > a system test out of these steps or more simplified ones without conntrack? > > A unit test would be ideal, though. > > I didn't review the patch itself, but the issue seems not easy to analyze > and very easy to re-introduce someday without regression tests. > > Best regards, Ilya Maximets. Thanks William and Ilya for feedback. WIll work with Toms to add a test, then submit the next version. Thanks, -Yi-Hung
On 12/17/20 9:23 PM, Yi-Hung Wei wrote: > On Wed, Dec 16, 2020 at 1:24 AM Ilya Maximets <i.maximets@ovn.org> wrote: >> >> On 12/16/20 12:41 AM, Yi-Hung Wei wrote: >>> On userspace datapath, geneve option flag FLOW_TNL_F_UDPIF should only >>> be set when the geneve option is available. However, currently, we always >>> set FLOW_TNL_F_UDPIF in the metadata flag, and that may result in megaflow >>> revalidation issue when the geneve option length is zero due to the datapath >>> flow key mis-match. That is the original flow key always has FLOW_TNL_F_UDPIF >>> set, while the regenerated flow key during revalidation process only has >>> FLOW_TNL_F_UDPIF set if geneve option length is zero. For reference, check >>> tun_metadata_to_geneve_nlattr() to see how the flow key of geneve >>> tunnel metadatafor are generated from netlink attributes. >>> >>> The following are the reproducible steps reported by Antonin. >>> After step 6, OvS reports a warning about failing to update the datapath >>> flow. >>> >>> 2020-12-15T01:56:21.324Z|00007|dpif(revalidator4)|WARN|netdev@ovs-netdev: >>> failed to put[modify] (No such file or directory) >>> ufid:d252fe62-5b2a-44e6-846a-190328190e09 skb_priority(0/0), >>> tunnel(tun_id=0x0,src=192.168.77.1,dst=192.168.77.2,ttl=64/0,tp_src=57391/0, >>> tp_dst=6081/0,flags(-df-csum+key)),skb_mark(0/0),ct_state(0x21/0x21), >>> ct_zone(0xfff0/0),ct_mark(0/0),ct_label(0/0),ct_tuple4(src=10.10.0.1/0.0.0.0, >>> dst=10.10.0.2/0.0.0.0,proto=1/0,tp_src=8/0,tp_dst=0/0),recirc_id(0xa), >>> dp_hash(0/0),in_port(5),packet_type(ns=0,id=0), >>> eth(src=1e:9c:f0:fb:18:9c/00:00:00:00:00:00,dst=ee:9f:60:8a:c0:a5/00:00:00:00:00:00), >>> eth_type(0x0800),ipv4(src=10.10.0.1/0.0.0.0,dst=10.10.0.2,proto=1/0,tos=0/0, >>> ttl=64/0,frag=no),icmp(type=8/0,code=0/0), actions:drop >>> >>> Setup: >>> 2 Nodes on the same subnet (192.168.77.101/24 - 192.168.77.102/24) >>> >>> Step 1 – Creating bridges (run on each Node): >>> >>> iface="enp0s8" >>> >>> hwaddr=$(ip link show $iface | grep link/ether | awk '{print $2}') >>> inet=$(ip addr show $iface | grep "inet " | awk '{ print $2 }') >>> >>> ovs-vsctl add-br br-phy \ >>> -- set Bridge br-phy datapath_type=netdev \ >>> -- br-set-external-id br-phy bridge-id br-phy \ >>> -- set bridge br-phy fail-mode=standalone \ >>> other_config:hwaddr="$hwaddr" >>> ovs-vsctl --timeout 10 add-port br-phy "$iface" >>> ip addr add "$inet" dev br-phy >>> ip link set br-phy up >>> ip addr flush dev enp0s8 2>/dev/null >>> ip link set enp0s8 up >>> iptables -F >>> >>> ovs-vsctl add-br br-int \ >>> -- set Bridge br-int datapath_type=netdev \ >>> -- set bridge br-phy fail-mode=standalone >>> >>> Step 2 – Creating netns to for overlay endpoints: >>> >>> On first Node (192.168.77.101): >>> ip netns add ns0 >>> ip link add veth0 type veth peer name veth1 >>> ip link set veth0 netns ns0 >>> ovs-vsctl add-port br-int veth1 >>> ip link set dev veth1 up >>> ip netns exec ns0 ip addr add 10.10.0.1/24 dev veth0 >>> ip netns exec ns0 ip link set dev veth0 up >>> >>> On second Node (192.168.77.102): >>> ip netns add ns0 >>> ip link add veth0 type veth peer name veth1 >>> ip link set veth0 netns ns0 >>> ovs-vsctl add-port br-int veth1 >>> ip link set dev veth1 up >>> ip netns exec ns0 ip addr add 10.10.0.2/24 dev veth0 >>> ip netns exec ns0 ip link set dev veth0 up >>> >>> Step 3 – Create tunnel (run on each Node): >>> >>> ovs-vsctl add-port br-int tun0 -- set interface tun0 type=geneve \ >>> ofport_request=100 options:remote_ip=flow options:key=flow >>> >>> Step 4 – Create the following flows: >>> >>> On first Node (192.168.77.101): >>> root@ovs-test-node-1:/home/vagrant# ovs-ofctl dump-flows br-int --no-stats >>> priority=100,ip actions=resubmit(,10) >>> priority=0 actions=NORMAL >>> priority=50 actions=resubmit(,40) >>> table=10, priority=100,ip actions=ct(table=20,zone=65520) >>> table=20, priority=200,ct_state=-new+trk,ip actions=resubmit(,30) >>> table=20, priority=100,ip,nw_dst=10.10.0.2 actions=resubmit(,30) >>> table=20, priority=0,ip actions=drop >>> table=30, priority=100,ip actions=ct(commit,table=40,zone=65520) >>> table=40, priority=100,in_port=veth1 actions=load:0xc0a84d66->NXM_NX_TUN_IPV4_DST[],output:tun0 >>> table=40, priority=100,in_port=tun0 actions=output:veth1 >>> table=40, priority=0 actions=drop >>> >>> On second Node (192.168.77.102): >>> root@ovs-test-node-2:/home/vagrant# ovs-ofctl dump-flows br-int --no-stats >>> priority=100,ip actions=resubmit(,10) >>> priority=0 actions=NORMAL >>> priority=50 actions=resubmit(,40) >>> table=10, priority=100,ip actions=ct(table=20,zone=65520) >>> table=20, priority=200,ct_state=-new+trk,ip actions=resubmit(,30) >>> table=20, priority=100,ip,nw_dst=10.10.0.2 actions=resubmit(,30) >>> table=20, priority=0,ip actions=drop >>> table=30, priority=100,ip actions=ct(commit,table=40,zone=65520) >>> table=40, priority=100,in_port=veth1 actions=load:0xc0a84d65->NXM_NX_TUN_IPV4_DST[],output:tun0 >>> table=40, priority=100,in_port=tun0 actions=output:veth1 >>> table=40, priority=0 actions=drop >>> >>> Step 5 – Check that ping works: >>> From the first Node: ip netns exec ns0 ping 10.10.0.2 >>> >>> Step 6 – The actual issue: >>> From the first Node: ip netns exec ns0 ping -c 3 10.10.0.2 >>> From the second Node: ovs-ofctl del-flows br-int 'table=20,ip,nw_dst=10.10.0.2' >>> From the first Node: ip netns exec ns0 ping -c 3 10.10.0.2 >>> >>> Execute these instructions in order, as soon as the previous one >>> completes. If you follow these steps, you should see the ping in >>> the last step succeed. This is not expected because of the deleted >>> flow. It also does not happen with VXLAN. >>> >> >> Since you already know how to reproduce the issue, could you, please, create >> a system test out of these steps or more simplified ones without conntrack? >> >> A unit test would be ideal, though. >> >> I didn't review the patch itself, but the issue seems not easy to analyze >> and very easy to re-introduce someday without regression tests. >> >> Best regards, Ilya Maximets. > > Thanks William and Ilya for feedback. WIll work with Toms to add a > test, then submit the next version. Hi! Is there any progress on this patch? Similar patches are getting submitted to the mail-list: http://patchwork.ozlabs.org/project/openvswitch/patch/20210204102338.128960-1-salems@nvidia.com/ Best regards, Ilya Maximets.
The patch is ready, I am writing a test for this. Maybe it was worth checking this in and adding test later!? On Thu, Feb 4, 2021 at 6:20 AM Ilya Maximets <i.maximets@ovn.org> wrote: > On 12/17/20 9:23 PM, Yi-Hung Wei wrote: > > On Wed, Dec 16, 2020 at 1:24 AM Ilya Maximets <i.maximets@ovn.org> > wrote: > >> > >> On 12/16/20 12:41 AM, Yi-Hung Wei wrote: > >>> On userspace datapath, geneve option flag FLOW_TNL_F_UDPIF should only > >>> be set when the geneve option is available. However, currently, we > always > >>> set FLOW_TNL_F_UDPIF in the metadata flag, and that may result in > megaflow > >>> revalidation issue when the geneve option length is zero due to the > datapath > >>> flow key mis-match. That is the original flow key always has > FLOW_TNL_F_UDPIF > >>> set, while the regenerated flow key during revalidation process only > has > >>> FLOW_TNL_F_UDPIF set if geneve option length is zero. For reference, > check > >>> tun_metadata_to_geneve_nlattr() to see how the flow key of geneve > >>> tunnel metadatafor are generated from netlink attributes. > >>> > >>> The following are the reproducible steps reported by Antonin. > >>> After step 6, OvS reports a warning about failing to update the > datapath > >>> flow. > >>> > >>> > 2020-12-15T01:56:21.324Z|00007|dpif(revalidator4)|WARN|netdev@ovs-netdev: > >>> failed to put[modify] (No such file or directory) > >>> ufid:d252fe62-5b2a-44e6-846a-190328190e09 skb_priority(0/0), > >>> > tunnel(tun_id=0x0,src=192.168.77.1,dst=192.168.77.2,ttl=64/0,tp_src=57391/0, > >>> tp_dst=6081/0,flags(-df-csum+key)),skb_mark(0/0),ct_state(0x21/0x21), > >>> ct_zone(0xfff0/0),ct_mark(0/0),ct_label(0/0),ct_tuple4(src= > 10.10.0.1/0.0.0.0, > >>> dst=10.10.0.2/0.0.0.0,proto=1/0,tp_src=8/0,tp_dst=0/0),recirc_id(0xa), > >>> dp_hash(0/0),in_port(5),packet_type(ns=0,id=0), > >>> > eth(src=1e:9c:f0:fb:18:9c/00:00:00:00:00:00,dst=ee:9f:60:8a:c0:a5/00:00:00:00:00:00), > >>> eth_type(0x0800),ipv4(src= > 10.10.0.1/0.0.0.0,dst=10.10.0.2,proto=1/0,tos=0/0, > >>> ttl=64/0,frag=no),icmp(type=8/0,code=0/0), actions:drop > >>> > >>> Setup: > >>> 2 Nodes on the same subnet (192.168.77.101/24 - 192.168.77.102/24) > >>> > >>> Step 1 – Creating bridges (run on each Node): > >>> > >>> iface="enp0s8" > >>> > >>> hwaddr=$(ip link show $iface | grep link/ether | awk '{print $2}') > >>> inet=$(ip addr show $iface | grep "inet " | awk '{ print $2 }') > >>> > >>> ovs-vsctl add-br br-phy \ > >>> -- set Bridge br-phy datapath_type=netdev \ > >>> -- br-set-external-id br-phy bridge-id br-phy \ > >>> -- set bridge br-phy fail-mode=standalone \ > >>> other_config:hwaddr="$hwaddr" > >>> ovs-vsctl --timeout 10 add-port br-phy "$iface" > >>> ip addr add "$inet" dev br-phy > >>> ip link set br-phy up > >>> ip addr flush dev enp0s8 2>/dev/null > >>> ip link set enp0s8 up > >>> iptables -F > >>> > >>> ovs-vsctl add-br br-int \ > >>> -- set Bridge br-int datapath_type=netdev \ > >>> -- set bridge br-phy fail-mode=standalone > >>> > >>> Step 2 – Creating netns to for overlay endpoints: > >>> > >>> On first Node (192.168.77.101): > >>> ip netns add ns0 > >>> ip link add veth0 type veth peer name veth1 > >>> ip link set veth0 netns ns0 > >>> ovs-vsctl add-port br-int veth1 > >>> ip link set dev veth1 up > >>> ip netns exec ns0 ip addr add 10.10.0.1/24 dev veth0 > >>> ip netns exec ns0 ip link set dev veth0 up > >>> > >>> On second Node (192.168.77.102): > >>> ip netns add ns0 > >>> ip link add veth0 type veth peer name veth1 > >>> ip link set veth0 netns ns0 > >>> ovs-vsctl add-port br-int veth1 > >>> ip link set dev veth1 up > >>> ip netns exec ns0 ip addr add 10.10.0.2/24 dev veth0 > >>> ip netns exec ns0 ip link set dev veth0 up > >>> > >>> Step 3 – Create tunnel (run on each Node): > >>> > >>> ovs-vsctl add-port br-int tun0 -- set interface tun0 type=geneve \ > >>> ofport_request=100 options:remote_ip=flow options:key=flow > >>> > >>> Step 4 – Create the following flows: > >>> > >>> On first Node (192.168.77.101): > >>> root@ovs-test-node-1:/home/vagrant# ovs-ofctl dump-flows br-int > --no-stats > >>> priority=100,ip actions=resubmit(,10) > >>> priority=0 actions=NORMAL > >>> priority=50 actions=resubmit(,40) > >>> table=10, priority=100,ip actions=ct(table=20,zone=65520) > >>> table=20, priority=200,ct_state=-new+trk,ip actions=resubmit(,30) > >>> table=20, priority=100,ip,nw_dst=10.10.0.2 actions=resubmit(,30) > >>> table=20, priority=0,ip actions=drop > >>> table=30, priority=100,ip actions=ct(commit,table=40,zone=65520) > >>> table=40, priority=100,in_port=veth1 > actions=load:0xc0a84d66->NXM_NX_TUN_IPV4_DST[],output:tun0 > >>> table=40, priority=100,in_port=tun0 actions=output:veth1 > >>> table=40, priority=0 actions=drop > >>> > >>> On second Node (192.168.77.102): > >>> root@ovs-test-node-2:/home/vagrant# ovs-ofctl dump-flows br-int > --no-stats > >>> priority=100,ip actions=resubmit(,10) > >>> priority=0 actions=NORMAL > >>> priority=50 actions=resubmit(,40) > >>> table=10, priority=100,ip actions=ct(table=20,zone=65520) > >>> table=20, priority=200,ct_state=-new+trk,ip actions=resubmit(,30) > >>> table=20, priority=100,ip,nw_dst=10.10.0.2 actions=resubmit(,30) > >>> table=20, priority=0,ip actions=drop > >>> table=30, priority=100,ip actions=ct(commit,table=40,zone=65520) > >>> table=40, priority=100,in_port=veth1 > actions=load:0xc0a84d65->NXM_NX_TUN_IPV4_DST[],output:tun0 > >>> table=40, priority=100,in_port=tun0 actions=output:veth1 > >>> table=40, priority=0 actions=drop > >>> > >>> Step 5 – Check that ping works: > >>> From the first Node: ip netns exec ns0 ping 10.10.0.2 > >>> > >>> Step 6 – The actual issue: > >>> From the first Node: ip netns exec ns0 ping -c 3 10.10.0.2 > >>> From the second Node: ovs-ofctl del-flows br-int > 'table=20,ip,nw_dst=10.10.0.2' > >>> From the first Node: ip netns exec ns0 ping -c 3 10.10.0.2 > >>> > >>> Execute these instructions in order, as soon as the previous one > >>> completes. If you follow these steps, you should see the ping in > >>> the last step succeed. This is not expected because of the deleted > >>> flow. It also does not happen with VXLAN. > >>> > >> > >> Since you already know how to reproduce the issue, could you, please, > create > >> a system test out of these steps or more simplified ones without > conntrack? > >> > >> A unit test would be ideal, though. > >> > >> I didn't review the patch itself, but the issue seems not easy to > analyze > >> and very easy to re-introduce someday without regression tests. > >> > >> Best regards, Ilya Maximets. > > > > Thanks William and Ilya for feedback. WIll work with Toms to add a > > test, then submit the next version. > > Hi! Is there any progress on this patch? > Similar patches are getting submitted to the mail-list: > > http://patchwork.ozlabs.org/project/openvswitch/patch/20210204102338.128960-1-salems@nvidia.com/ > > Best regards, Ilya Maximets. >
On Thu, Feb 4, 2021 at 10:08 AM Cpp Code <cpp.code.lv@gmail.com> wrote: > > The patch is ready, I am writing a test for this. Maybe it was worth checking this in and adding test later!? > I'd suggest having a test case together with the patch, since this isn't a trivial issue. William
On 2/4/21 7:19 PM, William Tu wrote: > On Thu, Feb 4, 2021 at 10:08 AM Cpp Code <cpp.code.lv@gmail.com> wrote: >> >> The patch is ready, I am writing a test for this. Maybe it was worth checking this in and adding test later!? >> > I'd suggest having a test case together with the patch, since this > isn't a trivial issue. I agree. We should not rush this. I asked just to check that this patch didn't fall through the cracks. Best regards, Ilya Maximets.
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c index b89dfdd52a86..09cff3a1fc7d 100644 --- a/lib/netdev-native-tnl.c +++ b/lib/netdev-native-tnl.c @@ -1015,9 +1015,11 @@ netdev_geneve_pop_header(struct dp_packet *packet) tnl->tun_id = htonll(ntohl(get_16aligned_be32(&gnh->vni)) >> 8); tnl->flags |= FLOW_TNL_F_KEY; - memcpy(tnl->metadata.opts.gnv, gnh->options, opts_len); - tnl->metadata.present.len = opts_len; - tnl->flags |= FLOW_TNL_F_UDPIF; + if (opts_len > 0) { + memcpy(tnl->metadata.opts.gnv, gnh->options, opts_len); + tnl->metadata.present.len = opts_len; + tnl->flags |= FLOW_TNL_F_UDPIF; + } packet->packet_type = htonl(PT_ETH); dp_packet_reset_packet(packet, hlen);
On userspace datapath, geneve option flag FLOW_TNL_F_UDPIF should only be set when the geneve option is available. However, currently, we always set FLOW_TNL_F_UDPIF in the metadata flag, and that may result in megaflow revalidation issue when the geneve option length is zero due to the datapath flow key mis-match. That is the original flow key always has FLOW_TNL_F_UDPIF set, while the regenerated flow key during revalidation process only has FLOW_TNL_F_UDPIF set if geneve option length is zero. For reference, check tun_metadata_to_geneve_nlattr() to see how the flow key of geneve tunnel metadatafor are generated from netlink attributes. The following are the reproducible steps reported by Antonin. After step 6, OvS reports a warning about failing to update the datapath flow. 2020-12-15T01:56:21.324Z|00007|dpif(revalidator4)|WARN|netdev@ovs-netdev: failed to put[modify] (No such file or directory) ufid:d252fe62-5b2a-44e6-846a-190328190e09 skb_priority(0/0), tunnel(tun_id=0x0,src=192.168.77.1,dst=192.168.77.2,ttl=64/0,tp_src=57391/0, tp_dst=6081/0,flags(-df-csum+key)),skb_mark(0/0),ct_state(0x21/0x21), ct_zone(0xfff0/0),ct_mark(0/0),ct_label(0/0),ct_tuple4(src=10.10.0.1/0.0.0.0, dst=10.10.0.2/0.0.0.0,proto=1/0,tp_src=8/0,tp_dst=0/0),recirc_id(0xa), dp_hash(0/0),in_port(5),packet_type(ns=0,id=0), eth(src=1e:9c:f0:fb:18:9c/00:00:00:00:00:00,dst=ee:9f:60:8a:c0:a5/00:00:00:00:00:00), eth_type(0x0800),ipv4(src=10.10.0.1/0.0.0.0,dst=10.10.0.2,proto=1/0,tos=0/0, ttl=64/0,frag=no),icmp(type=8/0,code=0/0), actions:drop Setup: 2 Nodes on the same subnet (192.168.77.101/24 - 192.168.77.102/24) Step 1 – Creating bridges (run on each Node): iface="enp0s8" hwaddr=$(ip link show $iface | grep link/ether | awk '{print $2}') inet=$(ip addr show $iface | grep "inet " | awk '{ print $2 }') ovs-vsctl add-br br-phy \ -- set Bridge br-phy datapath_type=netdev \ -- br-set-external-id br-phy bridge-id br-phy \ -- set bridge br-phy fail-mode=standalone \ other_config:hwaddr="$hwaddr" ovs-vsctl --timeout 10 add-port br-phy "$iface" ip addr add "$inet" dev br-phy ip link set br-phy up ip addr flush dev enp0s8 2>/dev/null ip link set enp0s8 up iptables -F ovs-vsctl add-br br-int \ -- set Bridge br-int datapath_type=netdev \ -- set bridge br-phy fail-mode=standalone Step 2 – Creating netns to for overlay endpoints: On first Node (192.168.77.101): ip netns add ns0 ip link add veth0 type veth peer name veth1 ip link set veth0 netns ns0 ovs-vsctl add-port br-int veth1 ip link set dev veth1 up ip netns exec ns0 ip addr add 10.10.0.1/24 dev veth0 ip netns exec ns0 ip link set dev veth0 up On second Node (192.168.77.102): ip netns add ns0 ip link add veth0 type veth peer name veth1 ip link set veth0 netns ns0 ovs-vsctl add-port br-int veth1 ip link set dev veth1 up ip netns exec ns0 ip addr add 10.10.0.2/24 dev veth0 ip netns exec ns0 ip link set dev veth0 up Step 3 – Create tunnel (run on each Node): ovs-vsctl add-port br-int tun0 -- set interface tun0 type=geneve \ ofport_request=100 options:remote_ip=flow options:key=flow Step 4 – Create the following flows: On first Node (192.168.77.101): root@ovs-test-node-1:/home/vagrant# ovs-ofctl dump-flows br-int --no-stats priority=100,ip actions=resubmit(,10) priority=0 actions=NORMAL priority=50 actions=resubmit(,40) table=10, priority=100,ip actions=ct(table=20,zone=65520) table=20, priority=200,ct_state=-new+trk,ip actions=resubmit(,30) table=20, priority=100,ip,nw_dst=10.10.0.2 actions=resubmit(,30) table=20, priority=0,ip actions=drop table=30, priority=100,ip actions=ct(commit,table=40,zone=65520) table=40, priority=100,in_port=veth1 actions=load:0xc0a84d66->NXM_NX_TUN_IPV4_DST[],output:tun0 table=40, priority=100,in_port=tun0 actions=output:veth1 table=40, priority=0 actions=drop On second Node (192.168.77.102): root@ovs-test-node-2:/home/vagrant# ovs-ofctl dump-flows br-int --no-stats priority=100,ip actions=resubmit(,10) priority=0 actions=NORMAL priority=50 actions=resubmit(,40) table=10, priority=100,ip actions=ct(table=20,zone=65520) table=20, priority=200,ct_state=-new+trk,ip actions=resubmit(,30) table=20, priority=100,ip,nw_dst=10.10.0.2 actions=resubmit(,30) table=20, priority=0,ip actions=drop table=30, priority=100,ip actions=ct(commit,table=40,zone=65520) table=40, priority=100,in_port=veth1 actions=load:0xc0a84d65->NXM_NX_TUN_IPV4_DST[],output:tun0 table=40, priority=100,in_port=tun0 actions=output:veth1 table=40, priority=0 actions=drop Step 5 – Check that ping works: From the first Node: ip netns exec ns0 ping 10.10.0.2 Step 6 – The actual issue: From the first Node: ip netns exec ns0 ping -c 3 10.10.0.2 From the second Node: ovs-ofctl del-flows br-int 'table=20,ip,nw_dst=10.10.0.2' From the first Node: ip netns exec ns0 ping -c 3 10.10.0.2 Execute these instructions in order, as soon as the previous one completes. If you follow these steps, you should see the ping in the last step succeed. This is not expected because of the deleted flow. It also does not happen with VXLAN. Reported-by: Antonin Bas <abas@vmware.com> Reported-at: https://github.com/vmware-tanzu/antrea/issues/897 Fixes: 6b241d645291 ("netdev-vport: Factor-out tunnel Push-pop code into separate module.") Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> --- lib/netdev-native-tnl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)