Re: [ovs-dev] [PATCH v2] netlink: removed incorrect optimization
On 6/2/2021 3:00 PM, Toms Atteka wrote:
This optimization caused FLOW_TNL_F_UDPIF flag not to be used in
hash calculation for geneve tunnel when revalidating flows which
resulted in different cache hash values and incorrect behaviour.
Added test to prevent regression.
Reported-at: https://github.com/vmware-tanzu/antrea/issues/897
Signed-off-by: Toms Atteka
Hi Toms,
I think the patch is fine but it should have a 'Fixes' tag.
Thanks,
- Greg
---
lib/tun-metadata.c | 2 +-
tests/system-traffic.at | 54 +
2 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c
index c0b0ae044..828db72f5 100644
--- a/lib/tun-metadata.c
+++ b/lib/tun-metadata.c
@@ -828,7 +828,7 @@ tun_metadata_to_geneve_nlattr(const struct flow_tnl *tun,
} else {
tun_metadata_to_geneve_nlattr_mask(key, tun, flow, b);
}
-} else if (flow->metadata.present.len || is_mask) {
+} else {
nl_msg_put_unspec(b, OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS,
tun->metadata.opts.gnv,
flow->metadata.present.len);
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index fb5b9a36d..483cc7e83 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -574,6 +574,60 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2
10.1.1.100 | FORMAT_PI
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([datapath - ping over geneve tunnel, delete flow regression])
+OVS_CHECK_GENEVE()
+
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-underlay])
+
+AT_DATA([flows.txt], [dnl
+priority=100,icmp actions=resubmit(,10)
+priority=0 actions=NORMAL
+table=10, priority=100, ip, actions=ct(table=20,zone=65520)
+table=20, priority=200, ip, ct_state=-new+trk, actions=resubmit(,30)
+table=20, priority=100, ip, ct_state=+new, actions=resubmit(,30)
+table=20, priority=50, ip, actions=DROP
+table=30, priority=100, ip, actions=ct(commit,table=40,zone=65520)
+table=40, actions=normal
+])
+
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"])
+
+ADD_NAMESPACES(at_ns0)
+
+dnl Set up underlay link from host into the namespace using veth pair.
+ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24")
+AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"])
+AT_CHECK([ip link set dev br-underlay up])
+
+dnl Set up tunnel endpoints on OVS outside the namespace and with a native
+dnl linux device inside the namespace.
+ADD_OVS_TUNNEL([geneve], [br0], [at_gnv0], [172.31.1.1], [10.1.1.100/24])
+ADD_NATIVE_TUNNEL([geneve], [ns_gnv0], [at_ns0], [172.31.1.100], [10.1.1.1/24],
+ [vni 0])
+
+dnl First, check the underlay
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 172.31.1.100 | FORMAT_PING],
[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+dnl ping over tunnel should work
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.100 | FORMAT_PING],
[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-ofctl del-flows br0 "ct_state=+new"])
+
+dnl ping should not go through after removal of the flow
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.100 | FORMAT_PING],
[0], [dnl
+7 packets transmitted, 0 received, 100% packet loss, time 0ms
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP(["/|ERR|/d
+/|WARN|/d"])
+AT_CLEANUP
+
AT_SETUP([datapath - flow resume with geneve tun_metadata])
OVS_CHECK_GENEVE()
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netlink: removed incorrect optimization
Bleep bloop. Greetings Toms Atteka, I am a robot and I have tried out your
patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line has trailing whitespace
#28 FILE: lib/tun-metadata.c:831:
} else {
Lines checked: 99, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email
acon...@redhat.com
Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] netlink: removed incorrect optimization
This optimization caused FLOW_TNL_F_UDPIF flag not to be used in
hash calculation for geneve tunnel when revalidating flows which
resulted in different cache hash values and incorrect behaviour.
Added test to prevent regression.
Reported-at: https://github.com/vmware-tanzu/antrea/issues/897
Signed-off-by: Toms Atteka
---
lib/tun-metadata.c | 2 +-
tests/system-traffic.at | 54 +
2 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c
index c0b0ae044..828db72f5 100644
--- a/lib/tun-metadata.c
+++ b/lib/tun-metadata.c
@@ -828,7 +828,7 @@ tun_metadata_to_geneve_nlattr(const struct flow_tnl *tun,
} else {
tun_metadata_to_geneve_nlattr_mask(key, tun, flow, b);
}
-} else if (flow->metadata.present.len || is_mask) {
+} else {
nl_msg_put_unspec(b, OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS,
tun->metadata.opts.gnv,
flow->metadata.present.len);
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index fb5b9a36d..483cc7e83 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -574,6 +574,60 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2
10.1.1.100 | FORMAT_PI
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([datapath - ping over geneve tunnel, delete flow regression])
+OVS_CHECK_GENEVE()
+
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-underlay])
+
+AT_DATA([flows.txt], [dnl
+priority=100,icmp actions=resubmit(,10)
+priority=0 actions=NORMAL
+table=10, priority=100, ip, actions=ct(table=20,zone=65520)
+table=20, priority=200, ip, ct_state=-new+trk, actions=resubmit(,30)
+table=20, priority=100, ip, ct_state=+new, actions=resubmit(,30)
+table=20, priority=50, ip, actions=DROP
+table=30, priority=100, ip, actions=ct(commit,table=40,zone=65520)
+table=40, actions=normal
+])
+
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"])
+
+ADD_NAMESPACES(at_ns0)
+
+dnl Set up underlay link from host into the namespace using veth pair.
+ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24")
+AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"])
+AT_CHECK([ip link set dev br-underlay up])
+
+dnl Set up tunnel endpoints on OVS outside the namespace and with a native
+dnl linux device inside the namespace.
+ADD_OVS_TUNNEL([geneve], [br0], [at_gnv0], [172.31.1.1], [10.1.1.100/24])
+ADD_NATIVE_TUNNEL([geneve], [ns_gnv0], [at_ns0], [172.31.1.100], [10.1.1.1/24],
+ [vni 0])
+
+dnl First, check the underlay
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 172.31.1.100 | FORMAT_PING],
[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+dnl ping over tunnel should work
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.100 | FORMAT_PING],
[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-ofctl del-flows br0 "ct_state=+new"])
+
+dnl ping should not go through after removal of the flow
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.100 | FORMAT_PING],
[0], [dnl
+7 packets transmitted, 0 received, 100% packet loss, time 0ms
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP(["/|ERR|/d
+/|WARN|/d"])
+AT_CLEANUP
+
AT_SETUP([datapath - flow resume with geneve tun_metadata])
OVS_CHECK_GENEVE()
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
