diff mbox series

[ovs-dev] ovn-controller: Fix empty address set parsing problem.

Message ID 1505023105-90302-1-git-send-email-zhouhan@gmail.com
State Accepted
Headers show
Series [ovs-dev] ovn-controller: Fix empty address set parsing problem. | expand

Commit Message

Han Zhou Sept. 10, 2017, 5:58 a.m. UTC 
When an address set is empty, current implementation will generate
an ovs flow that matches random things (and in most cases matching
everything) due to a problem in expression parser of constant set.
This patch fixes it by replacing the expression by a boolean false
when the set is empty, and adds tests cases accordingly.

Reported-by: Guru Shetty <guru@ovn.org>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html
Signed-off-by: Han Zhou <zhouhan@gmail.com>
---
 ovn/lib/expr.c   | 4 ++++
 tests/ovn.at     | 9 +++++++++
 tests/test-ovn.c | 2 ++
 3 files changed, 15 insertions(+)

Comments

Ben Pfaff Sept. 10, 2017, 5:56 p.m. UTC | #1 
On Sat, Sep 09, 2017 at 10:58:25PM -0700, Han Zhou wrote:
> When an address set is empty, current implementation will generate
> an ovs flow that matches random things (and in most cases matching
> everything) due to a problem in expression parser of constant set.
> This patch fixes it by replacing the expression by a boolean false
> when the set is empty, and adds tests cases accordingly.
> 
> Reported-by: Guru Shetty <guru@ovn.org>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html
> Signed-off-by: Han Zhou <zhouhan@gmail.com>
> ---
>  ovn/lib/expr.c   | 4 ++++
>  tests/ovn.at     | 9 +++++++++
>  tests/test-ovn.c | 2 ++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
> index 060e9ee..6a731de 100644
> --- a/ovn/lib/expr.c
> +++ b/ovn/lib/expr.c
> @@ -612,6 +612,10 @@ make_cmp(struct expr_context *ctx,
>          }
>      }
>  
> +    if (!cs->n_values) {
> +        e = expr_create_boolean(false);
> +        goto exit;
> +    }

Thanks for working on this.  I agree with everyone that this is an
important fix.

I think that it overlooks the behavior of !=.  Presumably, == and !=
should yield opposite results.  To get that behavior, we want:
        e = expr_create_boolean(r == EXPR_R_NE);
instead of:
        e = expr_create_boolean(false);

Here's a v2 that includes this change plus tests for !=.  I added myself
as coauthor; I hope that's OK.

Thanks,

Ben.

--8<--------------------------cut here-------------------------->8--

From: Han Zhou <zhouhan@gmail.com>
Date: Sat, 9 Sep 2017 22:58:25 -0700
Subject: [PATCH] ovn-controller: Fix empty address set parsing problem.

When an address set is empty, current implementation will generate
an ovs flow that matches random things (and in most cases matching
everything) due to a problem in expression parser of constant set.
This patch fixes it by replacing the expression by a boolean false
when the set is empty, and adds tests cases accordingly.

Reported-by: Guru Shetty <guru@ovn.org>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html
Signed-off-by: Han Zhou <zhouhan@gmail.com>
Co-authored-by: Ben Pfaff <blp@ovn.org>
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ovn/lib/expr.c   |  4 ++++
 tests/ovn.at     | 32 ++++++++++++++++++++++++++++++++
 tests/test-ovn.c |  2 ++
 3 files changed, 38 insertions(+)

diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 060e9ee3d088..79ff45762f65 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -612,6 +612,10 @@ make_cmp(struct expr_context *ctx,
         }
     }
 
+    if (!cs->n_values) {
+        e = expr_create_boolean(r == EXPR_R_NE);
+        goto exit;
+    }
     e = make_cmp__(f, r, &cs->values[0]);
     for (size_t i = 1; i < cs->n_values; i++) {
         e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND,
diff --git a/tests/ovn.at b/tests/ovn.at
index bb9999ce0b1b..f2035290f66e 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -623,6 +623,38 @@ dl_src=00:00:00:00:00:02
 dl_src=00:00:00:00:00:03
 dl_src=ba:be:be:ef:de:ad
 ])
+AT_CHECK([expr_to_flow 'ip4.src == {$set4}'], [0], [dnl
+(no flows)
+])
+AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set4}'], [0], [dnl
+ip,nw_src=1.2.3.4
+])
+AT_CHECK([expr_to_flow 'ip4.src == 1.2.3.4 || ip4.src == {$set4}'], [0], [dnl
+ip,nw_src=1.2.3.4
+])
+AT_CHECK([expr_to_flow 'ip4.src != {$set4}'], [0], [dnl
+
+])
+AT_CHECK([expr_to_flow 'ip4.src != {1.0.0.0/8, $set4}'], [0], [dnl
+ip,nw_src=0.0.0.0/1.0.0.0
+ip,nw_src=128.0.0.0/1
+ip,nw_src=16.0.0.0/16.0.0.0
+ip,nw_src=2.0.0.0/2.0.0.0
+ip,nw_src=32.0.0.0/32.0.0.0
+ip,nw_src=4.0.0.0/4.0.0.0
+ip,nw_src=64.0.0.0/64.0.0.0
+ip,nw_src=8.0.0.0/8.0.0.0
+])
+AT_CHECK([expr_to_flow 'ip4.src != 1.0.0.0/8 && ip4.src != {$set4}'], [0], [dnl
+ip,nw_src=0.0.0.0/1.0.0.0
+ip,nw_src=128.0.0.0/1
+ip,nw_src=16.0.0.0/16.0.0.0
+ip,nw_src=2.0.0.0/2.0.0.0
+ip,nw_src=32.0.0.0/32.0.0.0
+ip,nw_src=4.0.0.0/4.0.0.0
+ip,nw_src=64.0.0.0/64.0.0.0
+ip,nw_src=8.0.0.0/8.0.0.0
+])
 AT_CLEANUP
 
 AT_SETUP([ovn -- action parsing])
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 694bc794a923..148ce122d385 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -202,10 +202,12 @@ create_addr_sets(struct shash *addr_sets)
     static const char *const addrs3[] = {
         "00:00:00:00:00:01", "00:00:00:00:00:02", "00:00:00:00:00:03",
     };
+    static const char *const addrs4[] = {};
 
     expr_addr_sets_add(addr_sets, "set1", addrs1, 3);
     expr_addr_sets_add(addr_sets, "set2", addrs2, 3);
     expr_addr_sets_add(addr_sets, "set3", addrs3, 3);
+    expr_addr_sets_add(addr_sets, "set4", addrs4, 0);
 }
 
 static bool
Han Zhou Sept. 10, 2017, 6:10 p.m. UTC | #2 
On Sun, Sep 10, 2017 at 10:56 AM, Ben Pfaff <blp@ovn.org> wrote:
>
> On Sat, Sep 09, 2017 at 10:58:25PM -0700, Han Zhou wrote:
> > When an address set is empty, current implementation will generate
> > an ovs flow that matches random things (and in most cases matching
> > everything) due to a problem in expression parser of constant set.
> > This patch fixes it by replacing the expression by a boolean false
> > when the set is empty, and adds tests cases accordingly.
> >
> > Reported-by: Guru Shetty <guru@ovn.org>
> > Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html
> > Signed-off-by: Han Zhou <zhouhan@gmail.com>
> > ---
> >  ovn/lib/expr.c   | 4 ++++
> >  tests/ovn.at     | 9 +++++++++
> >  tests/test-ovn.c | 2 ++
> >  3 files changed, 15 insertions(+)
> >
> > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
> > index 060e9ee..6a731de 100644
> > --- a/ovn/lib/expr.c
> > +++ b/ovn/lib/expr.c
> > @@ -612,6 +612,10 @@ make_cmp(struct expr_context *ctx,
> >          }
> >      }
> >
> > +    if (!cs->n_values) {
> > +        e = expr_create_boolean(false);
> > +        goto exit;
> > +    }
>
> Thanks for working on this.  I agree with everyone that this is an
> important fix.
>
> I think that it overlooks the behavior of !=.  Presumably, == and !=
> should yield opposite results.  To get that behavior, we want:
>         e = expr_create_boolean(r == EXPR_R_NE);
> instead of:
>         e = expr_create_boolean(false);
>
> Here's a v2 that includes this change plus tests for !=.  I added myself
> as coauthor; I hope that's OK.
>
> Thanks,
>
> Ben.
>
> --8<--------------------------cut here-------------------------->8--
>
> From: Han Zhou <zhouhan@gmail.com>
> Date: Sat, 9 Sep 2017 22:58:25 -0700
> Subject: [PATCH] ovn-controller: Fix empty address set parsing problem.
>
> When an address set is empty, current implementation will generate
> an ovs flow that matches random things (and in most cases matching
> everything) due to a problem in expression parser of constant set.
> This patch fixes it by replacing the expression by a boolean false
> when the set is empty, and adds tests cases accordingly.
>
> Reported-by: Guru Shetty <guru@ovn.org>
> Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html
> Signed-off-by: Han Zhou <zhouhan@gmail.com>
> Co-authored-by: Ben Pfaff <blp@ovn.org>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>  ovn/lib/expr.c   |  4 ++++
>  tests/ovn.at     | 32 ++++++++++++++++++++++++++++++++
>  tests/test-ovn.c |  2 ++
>  3 files changed, 38 insertions(+)
>
> diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
> index 060e9ee3d088..79ff45762f65 100644
> --- a/ovn/lib/expr.c
> +++ b/ovn/lib/expr.c
> @@ -612,6 +612,10 @@ make_cmp(struct expr_context *ctx,
>          }
>      }
>
> +    if (!cs->n_values) {
> +        e = expr_create_boolean(r == EXPR_R_NE);
> +        goto exit;
> +    }
>      e = make_cmp__(f, r, &cs->values[0]);
>      for (size_t i = 1; i < cs->n_values; i++) {
>          e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND,
> diff --git a/tests/ovn.at b/tests/ovn.at
> index bb9999ce0b1b..f2035290f66e 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -623,6 +623,38 @@ dl_src=00:00:00:00:00:02
>  dl_src=00:00:00:00:00:03
>  dl_src=ba:be:be:ef:de:ad
>  ])
> +AT_CHECK([expr_to_flow 'ip4.src == {$set4}'], [0], [dnl
> +(no flows)
> +])
> +AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set4}'], [0], [dnl
> +ip,nw_src=1.2.3.4
> +])
> +AT_CHECK([expr_to_flow 'ip4.src == 1.2.3.4 || ip4.src == {$set4}'], [0],
[dnl
> +ip,nw_src=1.2.3.4
> +])
> +AT_CHECK([expr_to_flow 'ip4.src != {$set4}'], [0], [dnl
> +
> +])
> +AT_CHECK([expr_to_flow 'ip4.src != {1.0.0.0/8, $set4}'], [0], [dnl
> +ip,nw_src=0.0.0.0/1.0.0.0
> +ip,nw_src=128.0.0.0/1
> +ip,nw_src=16.0.0.0/16.0.0.0
> +ip,nw_src=2.0.0.0/2.0.0.0
> +ip,nw_src=32.0.0.0/32.0.0.0
> +ip,nw_src=4.0.0.0/4.0.0.0
> +ip,nw_src=64.0.0.0/64.0.0.0
> +ip,nw_src=8.0.0.0/8.0.0.0
> +])
> +AT_CHECK([expr_to_flow 'ip4.src != 1.0.0.0/8 && ip4.src != {$set4}'],
[0], [dnl
> +ip,nw_src=0.0.0.0/1.0.0.0
> +ip,nw_src=128.0.0.0/1
> +ip,nw_src=16.0.0.0/16.0.0.0
> +ip,nw_src=2.0.0.0/2.0.0.0
> +ip,nw_src=32.0.0.0/32.0.0.0
> +ip,nw_src=4.0.0.0/4.0.0.0
> +ip,nw_src=64.0.0.0/64.0.0.0
> +ip,nw_src=8.0.0.0/8.0.0.0
> +])
>  AT_CLEANUP
>
>  AT_SETUP([ovn -- action parsing])
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index 694bc794a923..148ce122d385 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -202,10 +202,12 @@ create_addr_sets(struct shash *addr_sets)
>      static const char *const addrs3[] = {
>          "00:00:00:00:00:01", "00:00:00:00:00:02", "00:00:00:00:00:03",
>      };
> +    static const char *const addrs4[] = {};
>
>      expr_addr_sets_add(addr_sets, "set1", addrs1, 3);
>      expr_addr_sets_add(addr_sets, "set2", addrs2, 3);
>      expr_addr_sets_add(addr_sets, "set3", addrs3, 3);
> +    expr_addr_sets_add(addr_sets, "set4", addrs4, 0);
>  }
>
>  static bool
> --
> 2.10.2
>

Ahh, I missed the case of "!=". Thanks Ben for the quick fix!

Han
Ben Pfaff Sept. 11, 2017, 3:14 p.m. UTC | #3 
On Sun, Sep 10, 2017 at 11:10:58AM -0700, Han Zhou wrote:
> On Sun, Sep 10, 2017 at 10:56 AM, Ben Pfaff <blp@ovn.org> wrote:
> >
> > On Sat, Sep 09, 2017 at 10:58:25PM -0700, Han Zhou wrote:
> > > When an address set is empty, current implementation will generate
> > > an ovs flow that matches random things (and in most cases matching
> > > everything) due to a problem in expression parser of constant set.
> > > This patch fixes it by replacing the expression by a boolean false
> > > when the set is empty, and adds tests cases accordingly.
> > >
> > > Reported-by: Guru Shetty <guru@ovn.org>
> > > Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html
> > > Signed-off-by: Han Zhou <zhouhan@gmail.com>
> > > ---
> > >  ovn/lib/expr.c   | 4 ++++
> > >  tests/ovn.at     | 9 +++++++++
> > >  tests/test-ovn.c | 2 ++
> > >  3 files changed, 15 insertions(+)
> > >
> > > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
> > > index 060e9ee..6a731de 100644
> > > --- a/ovn/lib/expr.c
> > > +++ b/ovn/lib/expr.c
> > > @@ -612,6 +612,10 @@ make_cmp(struct expr_context *ctx,
> > >          }
> > >      }
> > >
> > > +    if (!cs->n_values) {
> > > +        e = expr_create_boolean(false);
> > > +        goto exit;
> > > +    }
> >
> > Thanks for working on this.  I agree with everyone that this is an
> > important fix.
> >
> > I think that it overlooks the behavior of !=.  Presumably, == and !=
> > should yield opposite results.  To get that behavior, we want:
> >         e = expr_create_boolean(r == EXPR_R_NE);
> > instead of:
> >         e = expr_create_boolean(false);
> >
> > Here's a v2 that includes this change plus tests for !=.  I added myself
> > as coauthor; I hope that's OK.
> >
> > Thanks,
> >
> > Ben.
> >
> > --8<--------------------------cut here-------------------------->8--
> >
> > From: Han Zhou <zhouhan@gmail.com>
> > Date: Sat, 9 Sep 2017 22:58:25 -0700
> > Subject: [PATCH] ovn-controller: Fix empty address set parsing problem.
> >
> > When an address set is empty, current implementation will generate
> > an ovs flow that matches random things (and in most cases matching
> > everything) due to a problem in expression parser of constant set.
> > This patch fixes it by replacing the expression by a boolean false
> > when the set is empty, and adds tests cases accordingly.
> >
> > Reported-by: Guru Shetty <guru@ovn.org>
> > Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html
> > Signed-off-by: Han Zhou <zhouhan@gmail.com>
> > Co-authored-by: Ben Pfaff <blp@ovn.org>
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> >  ovn/lib/expr.c   |  4 ++++
> >  tests/ovn.at     | 32 ++++++++++++++++++++++++++++++++
> >  tests/test-ovn.c |  2 ++
> >  3 files changed, 38 insertions(+)
> >
> > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
> > index 060e9ee3d088..79ff45762f65 100644
> > --- a/ovn/lib/expr.c
> > +++ b/ovn/lib/expr.c
> > @@ -612,6 +612,10 @@ make_cmp(struct expr_context *ctx,
> >          }
> >      }
> >
> > +    if (!cs->n_values) {
> > +        e = expr_create_boolean(r == EXPR_R_NE);
> > +        goto exit;
> > +    }
> >      e = make_cmp__(f, r, &cs->values[0]);
> >      for (size_t i = 1; i < cs->n_values; i++) {
> >          e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND,
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index bb9999ce0b1b..f2035290f66e 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -623,6 +623,38 @@ dl_src=00:00:00:00:00:02
> >  dl_src=00:00:00:00:00:03
> >  dl_src=ba:be:be:ef:de:ad
> >  ])
> > +AT_CHECK([expr_to_flow 'ip4.src == {$set4}'], [0], [dnl
> > +(no flows)
> > +])
> > +AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set4}'], [0], [dnl
> > +ip,nw_src=1.2.3.4
> > +])
> > +AT_CHECK([expr_to_flow 'ip4.src == 1.2.3.4 || ip4.src == {$set4}'], [0],
> [dnl
> > +ip,nw_src=1.2.3.4
> > +])
> > +AT_CHECK([expr_to_flow 'ip4.src != {$set4}'], [0], [dnl
> > +
> > +])
> > +AT_CHECK([expr_to_flow 'ip4.src != {1.0.0.0/8, $set4}'], [0], [dnl
> > +ip,nw_src=0.0.0.0/1.0.0.0
> > +ip,nw_src=128.0.0.0/1
> > +ip,nw_src=16.0.0.0/16.0.0.0
> > +ip,nw_src=2.0.0.0/2.0.0.0
> > +ip,nw_src=32.0.0.0/32.0.0.0
> > +ip,nw_src=4.0.0.0/4.0.0.0
> > +ip,nw_src=64.0.0.0/64.0.0.0
> > +ip,nw_src=8.0.0.0/8.0.0.0
> > +])
> > +AT_CHECK([expr_to_flow 'ip4.src != 1.0.0.0/8 && ip4.src != {$set4}'],
> [0], [dnl
> > +ip,nw_src=0.0.0.0/1.0.0.0
> > +ip,nw_src=128.0.0.0/1
> > +ip,nw_src=16.0.0.0/16.0.0.0
> > +ip,nw_src=2.0.0.0/2.0.0.0
> > +ip,nw_src=32.0.0.0/32.0.0.0
> > +ip,nw_src=4.0.0.0/4.0.0.0
> > +ip,nw_src=64.0.0.0/64.0.0.0
> > +ip,nw_src=8.0.0.0/8.0.0.0
> > +])
> >  AT_CLEANUP
> >
> >  AT_SETUP([ovn -- action parsing])
> > diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> > index 694bc794a923..148ce122d385 100644
> > --- a/tests/test-ovn.c
> > +++ b/tests/test-ovn.c
> > @@ -202,10 +202,12 @@ create_addr_sets(struct shash *addr_sets)
> >      static const char *const addrs3[] = {
> >          "00:00:00:00:00:01", "00:00:00:00:00:02", "00:00:00:00:00:03",
> >      };
> > +    static const char *const addrs4[] = {};
> >
> >      expr_addr_sets_add(addr_sets, "set1", addrs1, 3);
> >      expr_addr_sets_add(addr_sets, "set2", addrs2, 3);
> >      expr_addr_sets_add(addr_sets, "set3", addrs3, 3);
> > +    expr_addr_sets_add(addr_sets, "set4", addrs4, 0);
> >  }
> >
> >  static bool
> > --
> > 2.10.2
> >
> 
> Ahh, I missed the case of "!=". Thanks Ben for the quick fix!

You're welcome.

I applied this to master, branch-2.8, branch-2.7, and branch-2.6.
diff mbox series

Patch

diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 060e9ee..6a731de 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -612,6 +612,10 @@  make_cmp(struct expr_context *ctx,
         }
     }
 
+    if (!cs->n_values) {
+        e = expr_create_boolean(false);
+        goto exit;
+    }
     e = make_cmp__(f, r, &cs->values[0]);
     for (size_t i = 1; i < cs->n_values; i++) {
         e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND,
diff --git a/tests/ovn.at b/tests/ovn.at
index bb9999c..5608555 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -623,6 +623,15 @@  dl_src=00:00:00:00:00:02
 dl_src=00:00:00:00:00:03
 dl_src=ba:be:be:ef:de:ad
 ])
+AT_CHECK([expr_to_flow 'ip4.src == {$set4}'], [0], [dnl
+(no flows)
+])
+AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set4}'], [0], [dnl
+ip,nw_src=1.2.3.4
+])
+AT_CHECK([expr_to_flow 'ip4.src == 1.2.3.4 || ip4.src == {$set4}'], [0], [dnl
+ip,nw_src=1.2.3.4
+])
 AT_CLEANUP
 
 AT_SETUP([ovn -- action parsing])
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 694bc79..148ce12 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -202,10 +202,12 @@  create_addr_sets(struct shash *addr_sets)
     static const char *const addrs3[] = {
         "00:00:00:00:00:01", "00:00:00:00:00:02", "00:00:00:00:00:03",
     };
+    static const char *const addrs4[] = {};
 
     expr_addr_sets_add(addr_sets, "set1", addrs1, 3);
     expr_addr_sets_add(addr_sets, "set2", addrs2, 3);
     expr_addr_sets_add(addr_sets, "set3", addrs3, 3);
+    expr_addr_sets_add(addr_sets, "set4", addrs4, 0);
 }
 
 static bool