diff mbox series

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

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

Commit Message

Numan Siddique Jan. 22, 2021, 8:33 a.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.")
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 controller/lflow.c |  7 +++++++
 tests/ovn.at       | 28 ++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

Comments

Dumitru Ceara Jan. 25, 2021, 2:15 p.m. UTC | #1 
On 1/22/21 9:33 AM, 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.")
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---

Hi Numan,

Good catch!

>   controller/lflow.c |  7 +++++++
>   tests/ovn.at       | 28 ++++++++++++++++++++++++++++
>   2 files changed, 35 insertions(+)
> 
> diff --git a/controller/lflow.c b/controller/lflow.c
> index c02585b1e..a9420a7c4 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -754,6 +754,13 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
>                                         &m->match, &conj, &lflow->header_.uuid);
>               ofpbuf_uninit(&conj);
>           }
> +
> +        if (m->match.wc.masks.conj_id) {
> +            /* Reset the conj_id back to relative conj id. If caching is
> +             * enabled, then processing of the expr match next time (due to
> +             * full recompute) will result in the wrong conj_id match flow. */
> +            m->match.flow.conj_id -= conj_id_ofs;
> +        }

While this works I'm not sure this is the best way to fix the issue. 
We're now making the add_matches_to_flow_table() aware of the lflow cache.

In my opinion the problem is that the matches we cache when storing 
entries of type LCACHE_T_MATCHES are not fully processed.

One option to fix that is to pass the current conj_ids_ofs to 
expr_to_matches() to make sure the generated match objects always use 
the "final" conj_id.

This is quite intrusive I guess.

An alternative is to add a new step, something like:

prepare_matches(matches, conj_id_ofs)
{
     for_each m in matches:
         // adjust m conj_id
         if (m->match.wc.masks.conj_id)
             m->match.flow.conj_id += conj_id_ofs;
         for_each conj in m->conjunctions:
             conj->id += conj_id_ofs;
}

We'd also have to remove the parts that update the conj_ids in 
add_matches_to_flow_table():

https://github.com/ovn-org/ovn/blob/a2d043d2ee473da8e4a835730d86f16a47fb096b/controller/lflow.c#L712

and

https://github.com/ovn-org/ovn/blob/master/controller/lflow.c#L747

Then we could call prepare_matches() before add_matches_to_flow_table() if:
1. flow cache disabled.
2. if flow cache is enabled and we're caching lflow matches.

Later, when cached matches are used (from step 2 above) we don't need to 
change them in any way and we can just call add_matches_to_flow_table() 
without having to worry about offsets.

What do you think?

Thanks,
Dumitru
Numan Siddique Jan. 25, 2021, 7:13 p.m. UTC | #2 
On Mon, Jan 25, 2021 at 7:46 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 1/22/21 9:33 AM, 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.")
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
>
> Hi Numan,
>
> Good catch!
>
> >   controller/lflow.c |  7 +++++++
> >   tests/ovn.at       | 28 ++++++++++++++++++++++++++++
> >   2 files changed, 35 insertions(+)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index c02585b1e..a9420a7c4 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -754,6 +754,13 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
> >                                         &m->match, &conj, &lflow->header_.uuid);
> >               ofpbuf_uninit(&conj);
> >           }
> > +
> > +        if (m->match.wc.masks.conj_id) {
> > +            /* Reset the conj_id back to relative conj id. If caching is
> > +             * enabled, then processing of the expr match next time (due to
> > +             * full recompute) will result in the wrong conj_id match flow. */
> > +            m->match.flow.conj_id -= conj_id_ofs;
> > +        }
>
> While this works I'm not sure this is the best way to fix the issue.
> We're now making the add_matches_to_flow_table() aware of the lflow cache.


Thanks for the comments.

If you see, In the lflow_expr cache we do store the conj_ids offset(s)
 associated with the logical flow. So I thought , storing the original
expr match
in the cache would make sense. And hence the patch reverts the changes
done to the expr match conj_id in the add_matches_to_flow_table().

But I do think your suggestion would make sense. So I submitted v2 as per
your suggestion. Please take a look.

Thanks
Numan


>
> In my opinion the problem is that the matches we cache when storing
> entries of type LCACHE_T_MATCHES are not fully processed.
>
> One option to fix that is to pass the current conj_ids_ofs to
> expr_to_matches() to make sure the generated match objects always use
> the "final" conj_id.
>
> This is quite intrusive I guess.
>
> An alternative is to add a new step, something like:
>
> prepare_matches(matches, conj_id_ofs)
> {
>      for_each m in matches:
>          // adjust m conj_id
>          if (m->match.wc.masks.conj_id)
>              m->match.flow.conj_id += conj_id_ofs;
>          for_each conj in m->conjunctions:
>              conj->id += conj_id_ofs;
> }
>
> We'd also have to remove the parts that update the conj_ids in
> add_matches_to_flow_table():
>
> https://github.com/ovn-org/ovn/blob/a2d043d2ee473da8e4a835730d86f16a47fb096b/controller/lflow.c#L712
>
> and
>
> https://github.com/ovn-org/ovn/blob/master/controller/lflow.c#L747
>
> Then we could call prepare_matches() before add_matches_to_flow_table() if:
> 1. flow cache disabled.
> 2. if flow cache is enabled and we're caching lflow matches.
>
> Later, when cached matches are used (from step 2 above) we don't need to
> change them in any way and we can just call add_matches_to_flow_table()
> without having to worry about offsets.
>
> What do you think?




>
>
> 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..a9420a7c4 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -754,6 +754,13 @@  add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
                                       &m->match, &conj, &lflow->header_.uuid);
             ofpbuf_uninit(&conj);
         }
+
+        if (m->match.wc.masks.conj_id) {
+            /* Reset the conj_id back to relative conj id. If caching is
+             * enabled, then processing of the expr match next time (due to
+             * full recompute) will result in the wrong conj_id match flow. */
+            m->match.flow.conj_id -= conj_id_ofs;
+        }
     }
 
     ofpbuf_uninit(&ofpacts);
diff --git a/tests/ovn.at b/tests/ovn.at
index 8f884241d..0e297d2f2 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