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

Ben Pfaff blp at ovn.org
Mon Sep 11 15:14:31 UTC 2017


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 at 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 at ovn.org>
> > > Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html
> > > Signed-off-by: Han Zhou <zhouhan at 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 at 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 at ovn.org>
> > Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html
> > Signed-off-by: Han Zhou <zhouhan at gmail.com>
> > Co-authored-by: Ben Pfaff <blp at ovn.org>
> > Signed-off-by: Ben Pfaff <blp at 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.


More information about the dev mailing list