From b52c176dd76bc0321d9c8bfe1c9b4832daf64835 Mon Sep 17 00:00:00 2001

From: Mark Michelson <mmichels@redhat.com>

Date: Fri, 25 Oct 2019 13:25:12 -0400

Subject: [PATCH 2/2] Combine conjunctions with identical matches into one

 flow.

As stated in previous commits, conjunctive matches have an issue where

it is possible to install multiple flows that have identical matches.

This results in ambiguity, and can lead to features (such as ACLs) not

functioning properly.

This change fixes the problem by combining conjunctions with identical

matches into a single flow. As an example, in the past we may have had

something like:

nw_dst=10.0.0.1 actions=conjunction(2,1/2)

nw_dst=10.0.0.1 actions=conjunction(3,1/2)

This commit changes this into

nw_dst=10.0.0.1 actions=conjunction(2,1/2),conjunction(3,1/2)

This way, there is only a single flow with the proscribed match, and

there is no ambiguity.

Signed-off-by: Mark Michelson <mmichels@redhat.com>

---

 ovn/controller/lflow.c  |  5 ++--

 ovn/controller/ofctrl.c | 74 +++++++++++++++++++++++++++++++++++++++++++++--------

 ovn/controller/ofctrl.h |  6 +++++

 tests/ovn.at        | 17 +++++-------

 4 files changed, 80 insertions(+), 22 deletions(-)

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c

index 24e3a521e..f34abcebc 100644

--- a/ovn/controller/lflow.c

+++ b/ovn/controller/lflow.c

@@ -734,8 +734,9 @@ consider_logical_flow(

                 dst->clause = src->clause;

                 dst->n_clauses = src->n_clauses;

             }

-            ofctrl_add_flow(flow_table, ptable, lflow->priority, 0, &m->match,

-                            &conj, &lflow->header_.uuid);

+

+            ofctrl_add_or_append_flow(flow_table, ptable, lflow->priority, 0,

+                                      &m->match, &conj, &lflow->header_.uuid);

             ofpbuf_uninit(&conj;;

         }

     }

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

index 3131baff0..10edd84fb 100644

--- a/ovn/controller/ofctrl.c

+++ b/ovn/controller/ofctrl.c

@@ -69,6 +69,11 @@ struct ovn_flow {

     uint64_t cookie;

 };

+static struct ovn_flow *ovn_flow_alloc(uint8_t table_id, uint16_t priority,

+                                       uint64_t cookie,

+                                       const struct match *match,

+                                       const struct ofpbuf *actions,

+                                       const struct uuid *sb_uuid);

 static uint32_t ovn_flow_match_hash(const struct ovn_flow *);

 static struct ovn_flow *ovn_flow_lookup(struct hmap *flow_table,

                                         const struct ovn_flow *target,

@@ -657,16 +662,8 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table,

                           const struct uuid *sb_uuid,

                           bool log_duplicate_flow)

 {

-    struct ovn_flow *f = xmalloc(sizeof *f);

-    f->table_id = table_id;

-    f->priority = priority;

-    minimatch_init(&f->match, match);

-    f->ofpacts = xmemdup(actions->data, actions->size);

-    f->ofpacts_len = actions->size;

-    f->sb_uuid = *sb_uuid;

-    f->match_hmap_node.hash = ovn_flow_match_hash(f);

-    f->uuid_hindex_node.hash = uuid_hash(&f->sb_uuid);

-    f->cookie = cookie;

+    struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match,

+                                        actions, sb_uuid);

     ovn_flow_log(f, "ofctrl_add_flow");

@@ -721,9 +718,66 @@ ofctrl_add_flow(struct ovn_desired_flow_table *desired_flows,

     ofctrl_check_and_add_flow(desired_flows, table_id, priority, cookie,

                               match, actions, sb_uuid, true);

 }

+

+void

+ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,

+                          uint8_t table_id, uint16_t priority, uint64_t cookie,

+                          const struct match *match,

+                          const struct ofpbuf *actions,

+                          const struct uuid *sb_uuid)

+{

+    struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match,

+                                        actions, sb_uuid);

+

+    ovn_flow_log(f, "ofctrl_add_or_append_flow");

+

+    struct ovn_flow *existing;

+    existing = ovn_flow_lookup(&desired_flows->match_flow_table, f, false);

+    if (existing) {

+        /* There's already a flow with this particular match. Append the

+         * action to that flow rather than adding a new flow

+         */

+        uint64_t compound_stub[64 / 8];

+        struct ofpbuf compound;

+        ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub));

+        ofpbuf_put(&compound, existing->ofpacts, existing->ofpacts_len);

+        ofpbuf_put(&compound, f->ofpacts, f->ofpacts_len);

+

+        free(existing->ofpacts);

+        existing->ofpacts = xmemdup(compound.data, compound.size);

+        existing->ofpacts_len = compound.size;

+

+        ofpbuf_uninit(&compound);

+        ovn_flow_destroy(f);

+    } else {

+        hmap_insert(&desired_flows->match_flow_table, &f->match_hmap_node,

+                    f->match_hmap_node.hash);

+        hindex_insert(&desired_flows->uuid_flow_table, &f->uuid_hindex_node,

+                      f->uuid_hindex_node.hash);

+    }

+}

 /* ovn_flow. */

+static struct ovn_flow *

+ovn_flow_alloc(uint8_t table_id, uint16_t priority, uint64_t cookie,

+               const struct match *match, const struct ofpbuf *actions,

+               const struct uuid *sb_uuid)

+{

+    struct ovn_flow *f = xmalloc(sizeof *f);

+    f->table_id = table_id;

+    f->priority = priority;

+    minimatch_init(&f->match, match);

+    f->ofpacts = xmemdup(actions->data, actions->size);

+    f->ofpacts_len = actions->size;

+    f->sb_uuid = *sb_uuid;

+    f->match_hmap_node.hash = ovn_flow_match_hash(f);

+    f->uuid_hindex_node.hash = uuid_hash(&f->sb_uuid);

+    f->cookie = cookie;

+

+    return f;

+}

+

 /* Returns a hash of the match key in 'f'. */

 static uint32_t

 ovn_flow_match_hash(const struct ovn_flow *f)

diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h

index 1e9ac16b9..21d2ce648 100644

--- a/ovn/controller/ofctrl.h

+++ b/ovn/controller/ofctrl.h

@@ -70,6 +70,12 @@ void ofctrl_add_flow(struct ovn_desired_flow_table *, uint8_t table_id,

                      const struct match *, const struct ofpbuf *ofpacts,

                      const struct uuid *);

+void ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,

+                               uint8_t table_id, uint16_t priority,

+                               uint64_t cookie, const struct match *match,

+                               const struct ofpbuf *actions,

+                               const struct uuid *sb_uuid);

+

 void ofctrl_remove_flows(struct ovn_desired_flow_table *, const struct uuid *);

 void ovn_desired_flow_table_init(struct ovn_desired_flow_table *);

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

index 641a646fc..50d8efeec 100644

--- a/tests/ovn.at

+++ b/tests/ovn.at

@@ -12247,7 +12247,7 @@ ovn-nbctl create Address_Set name=set1 \

 addresses=\"10.0.0.4\",\"10.0.0.5\",\"10.0.0.6\"

 ovn-nbctl create Address_Set name=set2 \

 addresses=\"10.0.0.7\",\"10.0.0.8\",\"10.0.0.9\"

-ovn-nbctl acl-add ls1 to-lport 1002 \

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

 'ip4 && ip4.src == $set1 && ip4.dst == $set1' allow

 ovn-nbctl acl-add ls1 to-lport 1001 \

 'ip4 && ip4.src == $set1 && ip4.dst == $set2' drop

@@ -12296,7 +12296,7 @@ cat 2.expected > expout

 $PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets

 AT_CHECK([cat 2.packets], [0], [expout])

-# There should be total of 12 flows present with conjunction action and 2 flows

+# There should be total of 9 flows present with conjunction action and 2 flows

 # with conj match. Eg.

 # table=44, priority=2002,conj_id=2,metadata=0x1 actions=resubmit(,45)

 # table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop

@@ -12306,14 +12306,11 @@ AT_CHECK([cat 2.packets], [0], [expout])

 # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(3,2/2)

 # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(3,2/2)

 # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(3,2/2)

-# priority=2002,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(2,1/2)

-# priority=2002,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(2,1/2)

-# priority=2002,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(2,1/2)

-# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(3,1/2)

-# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(3,1/2)

-# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(3,1/2)

-

-OVS_WAIT_UNTIL([test 12 = `as hv1 ovs-ofctl dump-flows br-int | \

+# priority=2002,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(2,1/2),conjunction(3,1/2)

+# priority=2002,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(2,1/2),conjunction(3,1/2)

+# priority=2002,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(2,1/2),conjunction(3,1/2)

+

+OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \

 grep conjunction | wc -l`])

 OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \

 grep conj_id | wc -l`])

-- 

2.14.5