diff mbox series

[ovs-dev,v2] ovn-controller: Fix wrong conj_id match flows when caching is enabled.

Message ID 20210125190855.2923279-1-numans@ovn.org
State Accepted
Headers show
Series [ovs-dev,v2] ovn-controller: Fix wrong conj_id match flows when caching is enabled. | expand

Commit Message

Numan Siddique Jan. 25, 2021, 7:08 p.m. UTC 
From: Numan Siddique <numans@ovn.org>

When the below ACL is added -
ovn-nbctl acl-add ls1 to-lport 3
  '(ip4.src==10.0.0.1 || ip4.src==10.0.0.2) &&
   (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow

ovn-controller installs the below OF flows

table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2)
table=45, priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)

When a full recompute is triggered, ovn-controller deletes the last
OF flow with the match conj_id=2 and adds the below OF flow

table=45, priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)

For subsequent recomputes, the conj_id keeps increasing by 1.

This disrupts the traffic which matches on conjuction action flows.

This patch fixes this issue.

Fixes: 1213bc8270("ovn-controller: Cache logical flow expr matches.")
Suggested-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 controller/lflow.c | 46 ++++++++++++++++++++++++++++++----------------
 tests/ovn.at       | 28 ++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 16 deletions(-)

Comments

Dumitru Ceara Jan. 27, 2021, 10:07 a.m. UTC | #1 
On 1/25/21 8:08 PM, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> When the below ACL is added -
> ovn-nbctl acl-add ls1 to-lport 3
>    '(ip4.src==10.0.0.1 || ip4.src==10.0.0.2) &&
>     (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow
> 
> ovn-controller installs the below OF flows
> 
> table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
> table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
> table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
> table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2)
> table=45, priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> 
> When a full recompute is triggered, ovn-controller deletes the last
> OF flow with the match conj_id=2 and adds the below OF flow
> 
> table=45, priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> 
> For subsequent recomputes, the conj_id keeps increasing by 1.
> 
> This disrupts the traffic which matches on conjuction action flows.
> 
> This patch fixes this issue.
> 
> Fixes: 1213bc8270("ovn-controller: Cache logical flow expr matches.")
> Suggested-by: Dumitru Ceara <dceara@redhat.com>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>   controller/lflow.c | 46 ++++++++++++++++++++++++++++++----------------
>   tests/ovn.at       | 28 ++++++++++++++++++++++++++++
>   2 files changed, 58 insertions(+), 16 deletions(-)
> 
> diff --git a/controller/lflow.c b/controller/lflow.c
> index c02585b1e..7bb3cdaeb 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -668,9 +668,8 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
>   static void
>   add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
>                             const struct sbrec_datapath_binding *dp,
> -                          struct hmap *matches, size_t conj_id_ofs,
> -                          uint8_t ptable, uint8_t output_ptable,
> -                          struct ofpbuf *ovnacts,
> +                          struct hmap *matches, uint8_t ptable,
> +                          uint8_t output_ptable, struct ofpbuf *ovnacts,
>                             bool ingress, struct lflow_ctx_in *l_ctx_in,
>                             struct lflow_ctx_out *l_ctx_out)
>   {
> @@ -708,9 +707,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
>       struct expr_match *m;
>       HMAP_FOR_EACH (m, hmap_node, matches) {
>           match_set_metadata(&m->match, htonll(dp->tunnel_key));
> -        if (m->match.wc.masks.conj_id) {
> -            m->match.flow.conj_id += conj_id_ofs;
> -        }
>           if (datapath_is_switch(dp)) {
>               unsigned int reg_index
>                   = (ingress ? MFF_LOG_INPORT : MFF_LOG_OUTPORT) - MFF_REG0;
> @@ -744,7 +740,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
>                   struct ofpact_conjunction *dst;
>   
>                   dst = ofpact_put_CONJUNCTION(&conj);
> -                dst->id = src->id + conj_id_ofs;
> +                dst->id = src->id;
>                   dst->clause = src->clause;
>                   dst->n_clauses = src->n_clauses;
>               }
> @@ -815,6 +811,22 @@ convert_match_to_expr(const struct sbrec_logical_flow *lflow,
>       return expr_simplify(e);
>   }
>   
> +static void
> +prepare_expr_matches(struct hmap *matches, uint32_t conj_id_ofs)

I think this fits better as a function in expr.c because it only updates 
internals of the expression matches, maybe renamed to 
expr_matches_prepare().

> +{
> +    struct expr_match *m;
> +    HMAP_FOR_EACH (m, hmap_node, matches) {
> +        if (m->match.wc.masks.conj_id) {
> +            m->match.flow.conj_id += conj_id_ofs;
> +        }
> +
> +        for (int i = 0; i < m->n; i++) {

Nit: s/int i/size_t i/

With these small issues addressed:

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

Thanks,
Dumitru
Numan Siddique Jan. 27, 2021, 12:17 p.m. UTC | #2 
On Wed, Jan 27, 2021 at 3:37 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 1/25/21 8:08 PM, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > When the below ACL is added -
> > ovn-nbctl acl-add ls1 to-lport 3
> >    '(ip4.src==10.0.0.1 || ip4.src==10.0.0.2) &&
> >     (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow
> >
> > ovn-controller installs the below OF flows
> >
> > table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
> > table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
> > table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
> > table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2)
> > table=45, priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> >
> > When a full recompute is triggered, ovn-controller deletes the last
> > OF flow with the match conj_id=2 and adds the below OF flow
> >
> > table=45, priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> >
> > For subsequent recomputes, the conj_id keeps increasing by 1.
> >
> > This disrupts the traffic which matches on conjuction action flows.
> >
> > This patch fixes this issue.
> >
> > Fixes: 1213bc8270("ovn-controller: Cache logical flow expr matches.")
> > Suggested-by: Dumitru Ceara <dceara@redhat.com>
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >   controller/lflow.c | 46 ++++++++++++++++++++++++++++++----------------
> >   tests/ovn.at       | 28 ++++++++++++++++++++++++++++
> >   2 files changed, 58 insertions(+), 16 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index c02585b1e..7bb3cdaeb 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -668,9 +668,8 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
> >   static void
> >   add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
> >                             const struct sbrec_datapath_binding *dp,
> > -                          struct hmap *matches, size_t conj_id_ofs,
> > -                          uint8_t ptable, uint8_t output_ptable,
> > -                          struct ofpbuf *ovnacts,
> > +                          struct hmap *matches, uint8_t ptable,
> > +                          uint8_t output_ptable, struct ofpbuf *ovnacts,
> >                             bool ingress, struct lflow_ctx_in *l_ctx_in,
> >                             struct lflow_ctx_out *l_ctx_out)
> >   {
> > @@ -708,9 +707,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
> >       struct expr_match *m;
> >       HMAP_FOR_EACH (m, hmap_node, matches) {
> >           match_set_metadata(&m->match, htonll(dp->tunnel_key));
> > -        if (m->match.wc.masks.conj_id) {
> > -            m->match.flow.conj_id += conj_id_ofs;
> > -        }
> >           if (datapath_is_switch(dp)) {
> >               unsigned int reg_index
> >                   = (ingress ? MFF_LOG_INPORT : MFF_LOG_OUTPORT) - MFF_REG0;
> > @@ -744,7 +740,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
> >                   struct ofpact_conjunction *dst;
> >
> >                   dst = ofpact_put_CONJUNCTION(&conj);
> > -                dst->id = src->id + conj_id_ofs;
> > +                dst->id = src->id;
> >                   dst->clause = src->clause;
> >                   dst->n_clauses = src->n_clauses;
> >               }
> > @@ -815,6 +811,22 @@ convert_match_to_expr(const struct sbrec_logical_flow *lflow,
> >       return expr_simplify(e);
> >   }
> >
> > +static void
> > +prepare_expr_matches(struct hmap *matches, uint32_t conj_id_ofs)
>
> I think this fits better as a function in expr.c because it only updates
> internals of the expression matches, maybe renamed to
> expr_matches_prepare().
>
> > +{
> > +    struct expr_match *m;
> > +    HMAP_FOR_EACH (m, hmap_node, matches) {
> > +        if (m->match.wc.masks.conj_id) {
> > +            m->match.flow.conj_id += conj_id_ofs;
> > +        }
> > +
> > +        for (int i = 0; i < m->n; i++) {
>
> Nit: s/int i/size_t i/
>
> With these small issues addressed:
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks for the review and comments. I addressed them and pushed the
patch to master, branch-20.12 and branch-20.09.

Thanks
Numan

>
> Thanks,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/controller/lflow.c b/controller/lflow.c
index c02585b1e..7bb3cdaeb 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -668,9 +668,8 @@  update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
 static void
 add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
                           const struct sbrec_datapath_binding *dp,
-                          struct hmap *matches, size_t conj_id_ofs,
-                          uint8_t ptable, uint8_t output_ptable,
-                          struct ofpbuf *ovnacts,
+                          struct hmap *matches, uint8_t ptable,
+                          uint8_t output_ptable, struct ofpbuf *ovnacts,
                           bool ingress, struct lflow_ctx_in *l_ctx_in,
                           struct lflow_ctx_out *l_ctx_out)
 {
@@ -708,9 +707,6 @@  add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
     struct expr_match *m;
     HMAP_FOR_EACH (m, hmap_node, matches) {
         match_set_metadata(&m->match, htonll(dp->tunnel_key));
-        if (m->match.wc.masks.conj_id) {
-            m->match.flow.conj_id += conj_id_ofs;
-        }
         if (datapath_is_switch(dp)) {
             unsigned int reg_index
                 = (ingress ? MFF_LOG_INPORT : MFF_LOG_OUTPORT) - MFF_REG0;
@@ -744,7 +740,7 @@  add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
                 struct ofpact_conjunction *dst;
 
                 dst = ofpact_put_CONJUNCTION(&conj);
-                dst->id = src->id + conj_id_ofs;
+                dst->id = src->id;
                 dst->clause = src->clause;
                 dst->n_clauses = src->n_clauses;
             }
@@ -815,6 +811,22 @@  convert_match_to_expr(const struct sbrec_logical_flow *lflow,
     return expr_simplify(e);
 }
 
+static void
+prepare_expr_matches(struct hmap *matches, uint32_t conj_id_ofs)
+{
+    struct expr_match *m;
+    HMAP_FOR_EACH (m, hmap_node, matches) {
+        if (m->match.wc.masks.conj_id) {
+            m->match.flow.conj_id += conj_id_ofs;
+        }
+
+        for (int i = 0; i < m->n; i++) {
+            struct cls_conjunction *src = &m->conjunctions[i];
+            src->id += conj_id_ofs;
+        }
+    }
+}
+
 static bool
 consider_logical_flow__(const struct sbrec_logical_flow *lflow,
                         const struct sbrec_datapath_binding *dp,
@@ -915,9 +927,9 @@  consider_logical_flow__(const struct sbrec_logical_flow *lflow,
             return true;
         }
 
-        add_matches_to_flow_table(lflow, dp, &matches, *l_ctx_out->conj_id_ofs,
-                                  ptable, output_ptable, &ovnacts, ingress,
-                                  l_ctx_in, l_ctx_out);
+        prepare_expr_matches(&matches, *l_ctx_out->conj_id_ofs);
+        add_matches_to_flow_table(lflow, dp, &matches, ptable, output_ptable,
+                                  &ovnacts, ingress, l_ctx_in, l_ctx_out);
 
         ovnacts_free(ovnacts.data, ovnacts.size);
         ofpbuf_uninit(&ovnacts);
@@ -930,10 +942,11 @@  consider_logical_flow__(const struct sbrec_logical_flow *lflow,
         lflow_cache_get(l_ctx_out->lflow_cache_map, lflow);
 
     if (lc && lc->type == LCACHE_T_MATCHES) {
-        /* 'matches' is cached. No need to do expr parsing.
+        /* 'matches' is cached. No need to do expr parsing and no need
+         * to call prepare_expr_matches() to update the conj ids.
          * Add matches to flow table and return. */
-        add_matches_to_flow_table(lflow, dp, lc->expr_matches, lc->conj_id_ofs,
-                                  ptable, output_ptable, &ovnacts, ingress,
+        add_matches_to_flow_table(lflow, dp, lc->expr_matches, ptable,
+                                  output_ptable, &ovnacts, ingress,
                                   l_ctx_in, l_ctx_out);
         ovnacts_free(ovnacts.data, ovnacts.size);
         ofpbuf_uninit(&ovnacts);
@@ -1009,10 +1022,11 @@  consider_logical_flow__(const struct sbrec_logical_flow *lflow,
         }
     }
 
+    prepare_expr_matches(matches, lc->conj_id_ofs);
+
     /* Encode OVN logical actions into OpenFlow. */
-    add_matches_to_flow_table(lflow, dp, matches, lc->conj_id_ofs,
-                              ptable, output_ptable, &ovnacts, ingress,
-                              l_ctx_in, l_ctx_out);
+    add_matches_to_flow_table(lflow, dp, matches, ptable, output_ptable,
+                              &ovnacts, ingress, l_ctx_in, l_ctx_out);
     ovnacts_free(ovnacts.data, ovnacts.size);
     ofpbuf_uninit(&ovnacts);
 
diff --git a/tests/ovn.at b/tests/ovn.at
index 718b2eec5..1161cac50 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -13884,6 +13884,34 @@  reset_pcap_file hv1-vif2 hv1/vif2
 rm -f 2.packets
 > 2.expected
 
+# Trigger recompute and make sure that the traffic still works as expected.
+as hv1 ovn-appctl -t ovn-controller recompute
+
+# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed.
+for src in `seq 1 2`; do
+    for dst in `seq 3 4`; do
+        sip=`ip_to_hex 10 0 0 $src`
+        dip=`ip_to_hex 10 0 0 $dst`
+
+        test_ip 1 f00000000001 f00000000002 $sip $dip 2
+    done
+done
+
+# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.5 should be dropped.
+dip=`ip_to_hex 10 0 0 5`
+for src in `seq 1 2`; do
+    sip=`ip_to_hex 10 0 0 $src`
+
+    test_ip 1 f00000000001 f00000000002 $sip $dip
+done
+
+cat 2.expected > expout
+$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets
+AT_CHECK([cat 2.packets], [0], [expout])
+reset_pcap_file hv1-vif2 hv1/vif2
+rm -f 2.packets
+> 2.expected
+
 # Add two less restrictive allow ACLs for src IP 10.0.0.1.
 ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1 || ip4.src==10.0.0.1' allow
 ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow