5f9769
From 11f75ad5bef3d2f6a9d72a8b27468fc3ccfc3d7e Mon Sep 17 00:00:00 2001
5f9769
From: Numan Siddique <numans@ovn.org>
5f9769
Date: Fri, 22 Jan 2021 13:52:29 +0530
5f9769
Subject: [PATCH] ovn-controller: Fix wrong conj_id match flows when caching is
5f9769
 enabled.
5f9769
5f9769
When the below ACL is added -
5f9769
ovn-nbctl acl-add ls1 to-lport 3
5f9769
  '(ip4.src==10.0.0.1 || ip4.src==10.0.0.2) &&
5f9769
   (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow
5f9769
5f9769
ovn-controller installs the below OF flows
5f9769
5f9769
table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
5f9769
table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
5f9769
table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
5f9769
table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2)
5f9769
table=45, priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
5f9769
5f9769
When a full recompute is triggered, ovn-controller deletes the last
5f9769
OF flow with the match conj_id=2 and adds the below OF flow
5f9769
5f9769
table=45, priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
5f9769
5f9769
For subsequent recomputes, the conj_id keeps increasing by 1.
5f9769
5f9769
This disrupts the traffic which matches on conjuction action flows.
5f9769
5f9769
This patch fixes this issue.
5f9769
5f9769
Fixes: 1213bc8270("ovn-controller: Cache logical flow expr matches.")
5f9769
Suggested-by: Dumitru Ceara <dceara@redhat.com>
5f9769
Acked-by: Dumitru Ceara <dceara@redhat.com>
5f9769
Signed-off-by: Numan Siddique <numans@ovn.org>
5f9769
5f9769
(cherry-picked from master commit c83294970c62f662015a7979b12250580bee3001)
5f9769
---
5f9769
 controller/lflow.c | 30 ++++++++++++++----------------
5f9769
 include/ovn/expr.h |  1 +
5f9769
 lib/expr.c         | 19 +++++++++++++++++++
5f9769
 tests/ovn.at       | 28 ++++++++++++++++++++++++++++
5f9769
 4 files changed, 62 insertions(+), 16 deletions(-)
5f9769
5f9769
diff --git a/controller/lflow.c b/controller/lflow.c
5f9769
index c02585b1e..9f6aece9a 100644
5f9769
--- a/controller/lflow.c
5f9769
+++ b/controller/lflow.c
5f9769
@@ -668,9 +668,8 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
5f9769
 static void
5f9769
 add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
5f9769
                           const struct sbrec_datapath_binding *dp,
5f9769
-                          struct hmap *matches, size_t conj_id_ofs,
5f9769
-                          uint8_t ptable, uint8_t output_ptable,
5f9769
-                          struct ofpbuf *ovnacts,
5f9769
+                          struct hmap *matches, uint8_t ptable,
5f9769
+                          uint8_t output_ptable, struct ofpbuf *ovnacts,
5f9769
                           bool ingress, struct lflow_ctx_in *l_ctx_in,
5f9769
                           struct lflow_ctx_out *l_ctx_out)
5f9769
 {
5f9769
@@ -708,9 +707,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
5f9769
     struct expr_match *m;
5f9769
     HMAP_FOR_EACH (m, hmap_node, matches) {
5f9769
         match_set_metadata(&m->match, htonll(dp->tunnel_key));
5f9769
-        if (m->match.wc.masks.conj_id) {
5f9769
-            m->match.flow.conj_id += conj_id_ofs;
5f9769
-        }
5f9769
         if (datapath_is_switch(dp)) {
5f9769
             unsigned int reg_index
5f9769
                 = (ingress ? MFF_LOG_INPORT : MFF_LOG_OUTPORT) - MFF_REG0;
5f9769
@@ -744,7 +740,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
5f9769
                 struct ofpact_conjunction *dst;
5f9769
 
5f9769
                 dst = ofpact_put_CONJUNCTION(&conj;;
5f9769
-                dst->id = src->id + conj_id_ofs;
5f9769
+                dst->id = src->id;
5f9769
                 dst->clause = src->clause;
5f9769
                 dst->n_clauses = src->n_clauses;
5f9769
             }
5f9769
@@ -915,9 +911,9 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
5f9769
             return true;
5f9769
         }
5f9769
 
5f9769
-        add_matches_to_flow_table(lflow, dp, &matches, *l_ctx_out->conj_id_ofs,
5f9769
-                                  ptable, output_ptable, &ovnacts, ingress,
5f9769
-                                  l_ctx_in, l_ctx_out);
5f9769
+        expr_matches_prepare(&matches, *l_ctx_out->conj_id_ofs);
5f9769
+        add_matches_to_flow_table(lflow, dp, &matches, ptable, output_ptable,
5f9769
+                                  &ovnacts, ingress, l_ctx_in, l_ctx_out);
5f9769
 
5f9769
         ovnacts_free(ovnacts.data, ovnacts.size);
5f9769
         ofpbuf_uninit(&ovnacts);
5f9769
@@ -930,10 +926,11 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
5f9769
         lflow_cache_get(l_ctx_out->lflow_cache_map, lflow);
5f9769
 
5f9769
     if (lc && lc->type == LCACHE_T_MATCHES) {
5f9769
-        /* 'matches' is cached. No need to do expr parsing.
5f9769
+        /* 'matches' is cached. No need to do expr parsing and no need
5f9769
+         * to call expr_matches_prepare() to update the conj ids.
5f9769
          * Add matches to flow table and return. */
5f9769
-        add_matches_to_flow_table(lflow, dp, lc->expr_matches, lc->conj_id_ofs,
5f9769
-                                  ptable, output_ptable, &ovnacts, ingress,
5f9769
+        add_matches_to_flow_table(lflow, dp, lc->expr_matches, ptable,
5f9769
+                                  output_ptable, &ovnacts, ingress,
5f9769
                                   l_ctx_in, l_ctx_out);
5f9769
         ovnacts_free(ovnacts.data, ovnacts.size);
5f9769
         ofpbuf_uninit(&ovnacts);
5f9769
@@ -1009,10 +1006,11 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
5f9769
         }
5f9769
     }
5f9769
 
5f9769
+    expr_matches_prepare(matches, lc->conj_id_ofs);
5f9769
+
5f9769
     /* Encode OVN logical actions into OpenFlow. */
5f9769
-    add_matches_to_flow_table(lflow, dp, matches, lc->conj_id_ofs,
5f9769
-                              ptable, output_ptable, &ovnacts, ingress,
5f9769
-                              l_ctx_in, l_ctx_out);
5f9769
+    add_matches_to_flow_table(lflow, dp, matches, ptable, output_ptable,
5f9769
+                              &ovnacts, ingress, l_ctx_in, l_ctx_out);
5f9769
     ovnacts_free(ovnacts.data, ovnacts.size);
5f9769
     ofpbuf_uninit(&ovnacts);
5f9769
 
5f9769
diff --git a/include/ovn/expr.h b/include/ovn/expr.h
5f9769
index 0a83ec7a8..c2c821818 100644
5f9769
--- a/include/ovn/expr.h
5f9769
+++ b/include/ovn/expr.h
5f9769
@@ -477,6 +477,7 @@ uint32_t expr_to_matches(const struct expr *,
5f9769
                          const void *aux,
5f9769
                          struct hmap *matches);
5f9769
 void expr_matches_destroy(struct hmap *matches);
5f9769
+void expr_matches_prepare(struct hmap *matches, uint32_t conj_id_ofs);
5f9769
 void expr_matches_print(const struct hmap *matches, FILE *);
5f9769
 
5f9769
 /* Action parsing helper. */
5f9769
diff --git a/lib/expr.c b/lib/expr.c
5f9769
index 4566d9110..796e88ac7 100644
5f9769
--- a/lib/expr.c
5f9769
+++ b/lib/expr.c
5f9769
@@ -3125,6 +3125,25 @@ expr_to_matches(const struct expr *expr,
5f9769
     return n_conjs;
5f9769
 }
5f9769
 
5f9769
+/* Prepares the expr matches in the hmap 'matches' by updating the
5f9769
+ * conj id offsets specified in 'conj_id_ofs'.
5f9769
+ */
5f9769
+void
5f9769
+expr_matches_prepare(struct hmap *matches, uint32_t conj_id_ofs)
5f9769
+{
5f9769
+    struct expr_match *m;
5f9769
+    HMAP_FOR_EACH (m, hmap_node, matches) {
5f9769
+        if (m->match.wc.masks.conj_id) {
5f9769
+            m->match.flow.conj_id += conj_id_ofs;
5f9769
+        }
5f9769
+
5f9769
+        for (size_t i = 0; i < m->n; i++) {
5f9769
+            struct cls_conjunction *src = &m->conjunctions[i];
5f9769
+            src->id += conj_id_ofs;
5f9769
+        }
5f9769
+    }
5f9769
+}
5f9769
+
5f9769
 /* Destroys all of the 'struct expr_match'es in 'matches', as well as the
5f9769
  * 'matches' hmap itself. */
5f9769
 void
5f9769
diff --git a/tests/ovn.at b/tests/ovn.at
5f9769
index e2d2d8a9d..b890592ae 100644
5f9769
--- a/tests/ovn.at
5f9769
+++ b/tests/ovn.at
5f9769
@@ -13824,6 +13824,34 @@ reset_pcap_file hv1-vif2 hv1/vif2
5f9769
 rm -f 2.packets
5f9769
 > 2.expected
5f9769
 
5f9769
+# Trigger recompute and make sure that the traffic still works as expected.
5f9769
+as hv1 ovn-appctl -t ovn-controller recompute
5f9769
+
5f9769
+# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed.
5f9769
+for src in `seq 1 2`; do
5f9769
+    for dst in `seq 3 4`; do
5f9769
+        sip=`ip_to_hex 10 0 0 $src`
5f9769
+        dip=`ip_to_hex 10 0 0 $dst`
5f9769
+
5f9769
+        test_ip 1 f00000000001 f00000000002 $sip $dip 2
5f9769
+    done
5f9769
+done
5f9769
+
5f9769
+# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.5 should be dropped.
5f9769
+dip=`ip_to_hex 10 0 0 5`
5f9769
+for src in `seq 1 2`; do
5f9769
+    sip=`ip_to_hex 10 0 0 $src`
5f9769
+
5f9769
+    test_ip 1 f00000000001 f00000000002 $sip $dip
5f9769
+done
5f9769
+
5f9769
+cat 2.expected > expout
5f9769
+$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets
5f9769
+AT_CHECK([cat 2.packets], [0], [expout])
5f9769
+reset_pcap_file hv1-vif2 hv1/vif2
5f9769
+rm -f 2.packets
5f9769
+> 2.expected
5f9769
+
5f9769
 # Add two less restrictive allow ACLs for src IP 10.0.0.1.
5f9769
 ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1 || ip4.src==10.0.0.1' allow
5f9769
 ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow
5f9769
-- 
5f9769
2.29.2
5f9769