[ovs-dev] [PATCH v4] netlink: removed incorrect optimization

Ansis ansisatteka at gmail.com
Tue Jun 15 03:22:41 UTC 2021


On Mon, Jun 7, 2021 at 1:31 PM Toms Atteka <cpp.code.lv at gmail.com> 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.
>
> CC: Jesse Gross <jesse at nicira.com>
> Fixes: 6728d578f64e ("dpif-netdev: Translate Geneve options per-flow, not per-packet.")
> Reported-at: https://github.com/vmware-tanzu/antrea/issues/897
> Signed-off-by: Toms Atteka <cpp.code.lv at gmail.com>
> ---
>  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..af0bcbde8 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 {

I reverted the line above, but the regression prevention test you
added below still seems to pass. So - does this regression prevention
test serve any purpose or am I doing something wrong? Here is proof:

 18: datapath - n ok
 19: datapath - flow resume with geneve tun_metadata^C
system-kmod-testsuite: WARNING: caught signal INT, bailing out
make: *** [Makefile:6871: check-kernel] Error 1
aatteka at laptop:/tmp/ovs$ git diff
diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c
index af0bcbde8..c0b0ae044 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 {
+    } else if (flow->metadata.present.len || is_mask) {
         nl_msg_put_unspec(b, OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS,
                           tun->metadata.opts.gnv,
                           flow->metadata.present.len);



>          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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list