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

[Ansis]

On Mon, Jun 14, 2021 at 10:22 PM Ansis <ansisatteka at gmail.com> wrote:
>
> 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

I take it back. I ran make check-kernel. I had to run make
check-system-userspace to see your regression test in action.

William, do you have any comments for this patch? You indicated you
wanted to look at it too.


>  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