From 9ac0a75fd1995dee44f80ba4d7856cbf7b0e456d Mon Sep 17 00:00:00 2001

From: Han Zhou <hzhou@ovn.org>

Date: Thu, 1 Oct 2020 22:49:04 -0700

Subject: [PATCH 1/7] ofctrl.c: Fix duplicated flow handling in I-P while

 merging opposite changes.

In a scenario in I-P when a desired flow is removed and an exactly same flow is

added back in the same iteration, the merge_tracked_flows() function will merge

the operation without firing any OpenFlow action. However, if there are

multiple desired flows (e.g. F1 and F2) matching the same installed flow, and

if the one being re-added (say, F1) is the one currently installed in OVS, the

current implementation will update the currently installed flow to F2 in the

data structure while the actual OVS flow is not updated (still F1). So far

there is still no problem, but the member desired_flow of the installed flow

is pointing to the wrong desired flow.

Now there is only one place where the desired_flow member is consulted, in

update_installed_flows_by_track() for handling a tracked *modified* flow. In

reality flow modification happens only when conjunction flows need to be

combined, which would never happen together with the above scenario of

merge_tracked_flows(). So this wrong desired_flow pointer doesn't cause any

real problem yet.

However, there is a place that can already utilize this active desired_flow

information, which is when handling a tracked flow deletion in

update_installed_flows_by_track(): we can check if the flow being deleted is

the currently active one installed in OVS. If not, we don't need to send and

OpenFlow modify to OVS. This optimization is added in this patch, apart from

fixing the problem of merge_tracked_flows(). Without the fix, this optimization

would cause the test case added in this patch fail.

Fixes: f4e508dd7 ("ofctrl.c: Merge opposite changes of tracked flows before installing.")

Acked-by: Dumitru Ceara <dceara@redhat.com>

Signed-off-by: Han Zhou <hzhou@ovn.org>

(cherry picked from upstream commit 9d2e8d32fb9865513b70408a665184a67564390d)

Change-Id: Ieb30dd8f4d07aae472c222920fcb773a1deace4d

---

 controller/ofctrl.c |  28 +++++++++++++-

 tests/ovn.at        | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++

 2 files changed, 134 insertions(+), 2 deletions(-)

diff --git a/controller/ofctrl.c b/controller/ofctrl.c

index 81a00c8..e725c00 100644

--- a/controller/ofctrl.c

+++ b/controller/ofctrl.c

@@ -788,6 +788,24 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type)

     }

 }

+/* Returns true if a desired flow is active (the one currently installed

+ * among the list of desired flows that are linked to the same installed

+ * flow). */

+static inline bool

+desired_flow_is_active(struct desired_flow *d)

+{

+    return (d->installed_flow && d->installed_flow->desired_flow == d);

+}

+

+/* Set a desired flow as the active one among the list of desired flows

+ * that are linked to the same installed flow. */

+static inline void

+desired_flow_set_active(struct desired_flow *d)

+{

+    ovs_assert(d->installed_flow);

+    d->installed_flow->desired_flow = d;

+}

+

 static void

 link_installed_to_desired(struct installed_flow *i, struct desired_flow *d)

 {

@@ -1740,6 +1758,8 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table)

             /* del_f must have been installed, otherwise it should have been

              * removed during track_flow_add_or_modify. */

             ovs_assert(del_f->installed_flow);

+

+            bool del_f_was_active = desired_flow_is_active(del_f);

             if (!f->installed_flow) {

                 /* f is not installed yet. */

                 struct installed_flow *i = del_f->installed_flow;

@@ -1751,6 +1771,9 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table)

                 ovs_assert(f->installed_flow == del_f->installed_flow);

                 unlink_installed_to_desired(del_f->installed_flow, del_f);

             }

+            if (del_f_was_active) {

+                desired_flow_set_active(f);

+            }

             hmap_remove(&deleted_flows, &del_f->match_hmap_node);

             ovs_list_remove(&del_f->track_list_node);

             desired_flow_destroy(del_f);

@@ -1778,6 +1801,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table,

             /* The desired flow was deleted */

             if (f->installed_flow) {

                 struct installed_flow *i = f->installed_flow;

+                bool was_active = desired_flow_is_active(f);

                 unlink_installed_to_desired(i, f);

                 if (!i->desired_flow) {

@@ -1786,7 +1810,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table,

                     hmap_remove(&installed_flows, &i->match_hmap_node);

                     installed_flow_destroy(i);

-                } else {

+                } else if (was_active) {

                     /* There are other desired flow(s) referencing this

                      * installed flow, so update the OVS flow for the new

                      * active flow (at least the cookie will be different,

@@ -1810,7 +1834,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table,

                 hmap_insert(&installed_flows, &new_node->match_hmap_node,

                             new_node->flow.hash);

                 link_installed_to_desired(new_node, f);

-            } else if (i->desired_flow == f) {

+            } else if (desired_flow_is_active(f)) {

                 /* The installed flow is installed for f, but f has change

                  * tracked, so it must have been modified. */

                 ovn_flow_log(&i->flow, "updating installed (tracked)");

diff --git a/tests/ovn.at b/tests/ovn.at

index e3f3153..6f1ab59 100644

--- a/tests/ovn.at

+++ b/tests/ovn.at

@@ -22266,6 +22266,114 @@ OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected])

 OVN_CLEANUP([hv1])

 AT_CLEANUP

+# This test cases tests a scenario of ACL confliction with address set update.

+# It is to cover a corner case when flows are re-processed in the I-P

+# iteration, combined with the scenario of conflicting ACLs.

+AT_SETUP([ovn -- conflict ACLs with address set])

+ovn_start

+

+ovn-nbctl ls-add ls1

+

+ovn-nbctl lsp-add ls1 lsp1 \

+-- lsp-set-addresses lsp1 "f0:00:00:00:00:01 10.0.0.1"

+

+ovn-nbctl lsp-add ls1 lsp2 \

+-- lsp-set-addresses lsp2 "f0:00:00:00:00:02 10.0.0.2"

+

+net_add n1

+sim_add hv1

+

+as hv1

+ovs-vsctl add-br br-phys

+ovn_attach n1 br-phys 192.168.0.1

+ovs-vsctl -- add-port br-int hv1-vif1 -- \

+    set interface hv1-vif1 external-ids:iface-id=lsp1 \

+    options:tx_pcap=hv1/vif1-tx.pcap \

+    options:rxq_pcap=hv1/vif1-rx.pcap \

+    ofport-request=1

+

+ovs-vsctl -- add-port br-int hv1-vif2 -- \

+    set interface hv1-vif2 external-ids:iface-id=lsp2 \

+    options:tx_pcap=hv1/vif2-tx.pcap \

+    options:rxq_pcap=hv1/vif2-rx.pcap \

+    ofport-request=2

+

+# Default drop

+ovn-nbctl acl-add ls1 to-lport 1000 \

+'outport == "lsp1" && ip4' drop

+

+# test_ip INPORT SRC_MAC DST_MAC SRC_IP DST_IP OUTPORT...

+#

+# This shell function causes an ip packet to be received on INPORT.

+# The packet's content has Ethernet destination DST and source SRC

+# (each exactly 12 hex digits) and Ethernet type ETHTYPE (4 hex digits).

+# The OUTPORTs (zero or more) list the VIFs on which the packet should

+# be received.  INPORT and the OUTPORTs are specified as logical switch

+# port numbers, e.g. 11 for vif11.

+test_ip() {

+    # This packet has bad checksums but logical L3 routing doesn't check.

+    local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5

+    local packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}\

+${dst_ip}0035111100080000

+    shift; shift; shift; shift; shift

+    as hv1 ovs-appctl netdev-dummy/receive hv1-vif$inport $packet

+    for outport; do

+        echo $packet >> $outport.expected

+    done

+}

+

+ip_to_hex() {

+    printf "%02x%02x%02x%02x" "$@"

+}

+

+reset_pcap_file() {

+    local iface=$1

+    local pcap_file=$2

+    ovs-vsctl -- set Interface $iface options:tx_pcap=dummy-tx.pcap \

+options:rxq_pcap=dummy-rx.pcap

+    rm -f ${pcap_file}*.pcap

+    ovs-vsctl -- set Interface $iface options:tx_pcap=${pcap_file}-tx.pcap \

+options:rxq_pcap=${pcap_file}-rx.pcap

+}

+

+# Create an address set

+ovn-nbctl create Address_Set name=as1 \

+addresses=\"10.0.0.2\",\"10.0.0.3\"

+

+# Create overlapping ACLs resulting in conflict desired OVS flows

+# Add ACL1 uses the address set

+ovn-nbctl --wait=hv acl-add ls1 to-lport 1001 \

+'outport == "lsp1" && ip4 && ip4.src == $as1' allow

+

+# Add ACL2 which uses a single IP, which shouldn't take effect because

+# when it is added incrementally there is already a conflict one installed.

+ovn-nbctl --wait=hv acl-add ls1 to-lport 1001 \

+'outport == "lsp1" && ip4 && ip4.src == 10.0.0.2' drop

+

+

+sip=`ip_to_hex 10 0 0 2`

+dip=`ip_to_hex 10 0 0 1`

+test_ip 2 f00000000002 f00000000001 $sip $dip 1

+OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected])

+

+# Update the address set which causes flow reprocessing but the OVS flow

+# for allowing 10.0.0.2 should keep unchanged

+ovn-nbctl --wait=hv set Address_Set as1 addresses=\"10.0.0.2\"

+

+test_ip 2 f00000000002 f00000000001 $sip $dip 1

+OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected])

+

+# Delete the ACL1 that has "allow" action

+ovn-nbctl acl-del ls1 to-lport 1001 \

+'outport == "lsp1" && ip4 && ip4.src == $as1'

+

+# ACL2 should take effect and packet should be dropped

+test_ip 2 f00000000002 f00000000001 $sip $dip

+OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected])

+

+OVN_CLEANUP([hv1])

+AT_CLEANUP

+

 AT_SETUP([ovn -- port bind/unbind change handling with conj flows - IPv6])

 ovn_start

-- 

1.8.3.1