[{"id":1765982,"web_url":"http://patchwork.ozlabs.org/comment/1765982/","msgid":"<20170910175657.GA6175@ovn.org>","list_archive_url":null,"date":"2017-09-10T17:56:57","subject":"Re: [ovs-dev] [PATCH] ovn-controller: Fix empty address set parsing\n\tproblem.","submitter":{"id":67603,"url":"http://patchwork.ozlabs.org/api/people/67603/","name":"Ben Pfaff","email":"blp@ovn.org"},"content":"On Sat, Sep 09, 2017 at 10:58:25PM -0700, Han Zhou wrote:\n> When an address set is empty, current implementation will generate\n> an ovs flow that matches random things (and in most cases matching\n> everything) due to a problem in expression parser of constant set.\n> This patch fixes it by replacing the expression by a boolean false\n> when the set is empty, and adds tests cases accordingly.\n> \n> Reported-by: Guru Shetty \n> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html\n> Signed-off-by: Han Zhou \n> ---\n> ovn/lib/expr.c | 4 ++++\n> tests/ovn.at | 9 +++++++++\n> tests/test-ovn.c | 2 ++\n> 3 files changed, 15 insertions(+)\n> \n> diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c\n> index 060e9ee..6a731de 100644\n> --- a/ovn/lib/expr.c\n> +++ b/ovn/lib/expr.c\n> @@ -612,6 +612,10 @@ make_cmp(struct expr_context *ctx,\n> }\n> }\n> \n> + if (!cs->n_values) {\n> + e = expr_create_boolean(false);\n> + goto exit;\n> + }\n\nThanks for working on this. I agree with everyone that this is an\nimportant fix.\n\nI think that it overlooks the behavior of !=. Presumably, == and !=\nshould yield opposite results. To get that behavior, we want:\n e = expr_create_boolean(r == EXPR_R_NE);\ninstead of:\n e = expr_create_boolean(false);\n\nHere's a v2 that includes this change plus tests for !=. I added myself\nas coauthor; I hope that's OK.\n\nThanks,\n\nBen.\n\n--8<--------------------------cut here-------------------------->8--\n\nFrom: Han Zhou \nDate: Sat, 9 Sep 2017 22:58:25 -0700\nSubject: [PATCH] ovn-controller: Fix empty address set parsing problem.\n\nWhen an address set is empty, current implementation will generate\nan ovs flow that matches random things (and in most cases matching\neverything) due to a problem in expression parser of constant set.\nThis patch fixes it by replacing the expression by a boolean false\nwhen the set is empty, and adds tests cases accordingly.\n\nReported-by: Guru Shetty \nReported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html\nSigned-off-by: Han Zhou \nCo-authored-by: Ben Pfaff \nSigned-off-by: Ben Pfaff \n---\n ovn/lib/expr.c | 4 ++++\n tests/ovn.at | 32 ++++++++++++++++++++++++++++++++\n tests/test-ovn.c | 2 ++\n 3 files changed, 38 insertions(+)\n\ndiff --git a/ovn/lib/expr.c b/ovn/lib/expr.c\nindex 060e9ee3d088..79ff45762f65 100644\n--- a/ovn/lib/expr.c\n+++ b/ovn/lib/expr.c\n@@ -612,6 +612,10 @@ make_cmp(struct expr_context *ctx,\n }\n }\n \n+ if (!cs->n_values) {\n+ e = expr_create_boolean(r == EXPR_R_NE);\n+ goto exit;\n+ }\n e = make_cmp__(f, r, &cs->values[0]);\n for (size_t i = 1; i < cs->n_values; i++) {\n e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND,\ndiff --git a/tests/ovn.at b/tests/ovn.at\nindex bb9999ce0b1b..f2035290f66e 100644\n--- a/tests/ovn.at\n+++ b/tests/ovn.at\n@@ -623,6 +623,38 @@ dl_src=00:00:00:00:00:02\n dl_src=00:00:00:00:00:03\n dl_src=ba:be:be:ef:de:ad\n ])\n+AT_CHECK([expr_to_flow 'ip4.src == {$set4}'], [0], [dnl\n+(no flows)\n+])\n+AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set4}'], [0], [dnl\n+ip,nw_src=1.2.3.4\n+])\n+AT_CHECK([expr_to_flow 'ip4.src == 1.2.3.4 || ip4.src == {$set4}'], [0], [dnl\n+ip,nw_src=1.2.3.4\n+])\n+AT_CHECK([expr_to_flow 'ip4.src != {$set4}'], [0], [dnl\n+\n+])\n+AT_CHECK([expr_to_flow 'ip4.src != {1.0.0.0/8, $set4}'], [0], [dnl\n+ip,nw_src=0.0.0.0/1.0.0.0\n+ip,nw_src=128.0.0.0/1\n+ip,nw_src=16.0.0.0/16.0.0.0\n+ip,nw_src=2.0.0.0/2.0.0.0\n+ip,nw_src=32.0.0.0/32.0.0.0\n+ip,nw_src=4.0.0.0/4.0.0.0\n+ip,nw_src=64.0.0.0/64.0.0.0\n+ip,nw_src=8.0.0.0/8.0.0.0\n+])\n+AT_CHECK([expr_to_flow 'ip4.src != 1.0.0.0/8 && ip4.src != {$set4}'], [0], [dnl\n+ip,nw_src=0.0.0.0/1.0.0.0\n+ip,nw_src=128.0.0.0/1\n+ip,nw_src=16.0.0.0/16.0.0.0\n+ip,nw_src=2.0.0.0/2.0.0.0\n+ip,nw_src=32.0.0.0/32.0.0.0\n+ip,nw_src=4.0.0.0/4.0.0.0\n+ip,nw_src=64.0.0.0/64.0.0.0\n+ip,nw_src=8.0.0.0/8.0.0.0\n+])\n AT_CLEANUP\n \n AT_SETUP([ovn -- action parsing])\ndiff --git a/tests/test-ovn.c b/tests/test-ovn.c\nindex 694bc794a923..148ce122d385 100644\n--- a/tests/test-ovn.c\n+++ b/tests/test-ovn.c\n@@ -202,10 +202,12 @@ create_addr_sets(struct shash *addr_sets)\n static const char *const addrs3[] = {\n \"00:00:00:00:00:01\", \"00:00:00:00:00:02\", \"00:00:00:00:00:03\",\n };\n+ static const char *const addrs4[] = {};\n \n expr_addr_sets_add(addr_sets, \"set1\", addrs1, 3);\n expr_addr_sets_add(addr_sets, \"set2\", addrs2, 3);\n expr_addr_sets_add(addr_sets, \"set3\", addrs3, 3);\n+ expr_addr_sets_add(addr_sets, \"set4\", addrs4, 0);\n }\n \n static bool","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xqzKV20d2z9s0Z\n\tfor ;\n\tMon, 11 Sep 2017 03:57:15 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id C7310ACC;\n\tSun, 10 Sep 2017 17:57:11 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 4E24AAB7\n\tfor ; Sun, 10 Sep 2017 17:57:10 +0000 (UTC)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id 1C118472\n\tfor ; Sun, 10 Sep 2017 17:57:06 +0000 (UTC)","from ovn.org (173-228-112-34.dsl.dynamic.fusionbroadband.com\n\t[173.228.112.34]) (Authenticated sender: blp@ovn.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 7AAA1FB886;\n\tSun, 10 Sep 2017 19:57:02 +0200 (CEST)"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-Originating-IP":"173.228.112.34","Date":"Sun, 10 Sep 2017 10:56:57 -0700","From":"Ben Pfaff ","To":"Han Zhou ","Message-ID":"<20170910175657.GA6175@ovn.org>","References":"<1505023105-90302-1-git-send-email-zhouhan@gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<1505023105-90302-1-git-send-email-zhouhan@gmail.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","X-Spam-Status":"No, score=-0.7 required=5.0 tests=RCVD_IN_DNSWL_LOW\n\tautolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Cc":"dev@openvswitch.org","Subject":"Re: [ovs-dev] [PATCH] ovn-controller: Fix empty address set parsing\n\tproblem.","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1765984,"web_url":"http://patchwork.ozlabs.org/comment/1765984/","msgid":"","list_archive_url":null,"date":"2017-09-10T18:10:58","subject":"Re: [ovs-dev] [PATCH] ovn-controller: Fix empty address set parsing\n\tproblem.","submitter":{"id":67381,"url":"http://patchwork.ozlabs.org/api/people/67381/","name":"Han Zhou","email":"zhouhan@gmail.com"},"content":"On Sun, Sep 10, 2017 at 10:56 AM, Ben Pfaff wrote:\n>\n> On Sat, Sep 09, 2017 at 10:58:25PM -0700, Han Zhou wrote:\n> > When an address set is empty, current implementation will generate\n> > an ovs flow that matches random things (and in most cases matching\n> > everything) due to a problem in expression parser of constant set.\n> > This patch fixes it by replacing the expression by a boolean false\n> > when the set is empty, and adds tests cases accordingly.\n> >\n> > Reported-by: Guru Shetty \n> > Reported-at:\nhttps://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html\n> > Signed-off-by: Han Zhou \n> > ---\n> > ovn/lib/expr.c | 4 ++++\n> > tests/ovn.at | 9 +++++++++\n> > tests/test-ovn.c | 2 ++\n> > 3 files changed, 15 insertions(+)\n> >\n> > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c\n> > index 060e9ee..6a731de 100644\n> > --- a/ovn/lib/expr.c\n> > +++ b/ovn/lib/expr.c\n> > @@ -612,6 +612,10 @@ make_cmp(struct expr_context *ctx,\n> > }\n> > }\n> >\n> > + if (!cs->n_values) {\n> > + e = expr_create_boolean(false);\n> > + goto exit;\n> > + }\n>\n> Thanks for working on this. I agree with everyone that this is an\n> important fix.\n>\n> I think that it overlooks the behavior of !=. Presumably, == and !=\n> should yield opposite results. To get that behavior, we want:\n> e = expr_create_boolean(r == EXPR_R_NE);\n> instead of:\n> e = expr_create_boolean(false);\n>\n> Here's a v2 that includes this change plus tests for !=. I added myself\n> as coauthor; I hope that's OK.\n>\n> Thanks,\n>\n> Ben.\n>\n> --8<--------------------------cut here-------------------------->8--\n>\n> From: Han Zhou \n> Date: Sat, 9 Sep 2017 22:58:25 -0700\n> Subject: [PATCH] ovn-controller: Fix empty address set parsing problem.\n>\n> When an address set is empty, current implementation will generate\n> an ovs flow that matches random things (and in most cases matching\n> everything) due to a problem in expression parser of constant set.\n> This patch fixes it by replacing the expression by a boolean false\n> when the set is empty, and adds tests cases accordingly.\n>\n> Reported-by: Guru Shetty \n> Reported-at:\nhttps://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html\n> Signed-off-by: Han Zhou \n> Co-authored-by: Ben Pfaff \n> Signed-off-by: Ben Pfaff \n> ---\n> ovn/lib/expr.c | 4 ++++\n> tests/ovn.at | 32 ++++++++++++++++++++++++++++++++\n> tests/test-ovn.c | 2 ++\n> 3 files changed, 38 insertions(+)\n>\n> diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c\n> index 060e9ee3d088..79ff45762f65 100644\n> --- a/ovn/lib/expr.c\n> +++ b/ovn/lib/expr.c\n> @@ -612,6 +612,10 @@ make_cmp(struct expr_context *ctx,\n> }\n> }\n>\n> + if (!cs->n_values) {\n> + e = expr_create_boolean(r == EXPR_R_NE);\n> + goto exit;\n> + }\n> e = make_cmp__(f, r, &cs->values[0]);\n> for (size_t i = 1; i < cs->n_values; i++) {\n> e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND,\n> diff --git a/tests/ovn.at b/tests/ovn.at\n> index bb9999ce0b1b..f2035290f66e 100644\n> --- a/tests/ovn.at\n> +++ b/tests/ovn.at\n> @@ -623,6 +623,38 @@ dl_src=00:00:00:00:00:02\n> dl_src=00:00:00:00:00:03\n> dl_src=ba:be:be:ef:de:ad\n> ])\n> +AT_CHECK([expr_to_flow 'ip4.src == {$set4}'], [0], [dnl\n> +(no flows)\n> +])\n> +AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set4}'], [0], [dnl\n> +ip,nw_src=1.2.3.4\n> +])\n> +AT_CHECK([expr_to_flow 'ip4.src == 1.2.3.4 || ip4.src == {$set4}'], [0],\n[dnl\n> +ip,nw_src=1.2.3.4\n> +])\n> +AT_CHECK([expr_to_flow 'ip4.src != {$set4}'], [0], [dnl\n> +\n> +])\n> +AT_CHECK([expr_to_flow 'ip4.src != {1.0.0.0/8, $set4}'], [0], [dnl\n> +ip,nw_src=0.0.0.0/1.0.0.0\n> +ip,nw_src=128.0.0.0/1\n> +ip,nw_src=16.0.0.0/16.0.0.0\n> +ip,nw_src=2.0.0.0/2.0.0.0\n> +ip,nw_src=32.0.0.0/32.0.0.0\n> +ip,nw_src=4.0.0.0/4.0.0.0\n> +ip,nw_src=64.0.0.0/64.0.0.0\n> +ip,nw_src=8.0.0.0/8.0.0.0\n> +])\n> +AT_CHECK([expr_to_flow 'ip4.src != 1.0.0.0/8 && ip4.src != {$set4}'],\n[0], [dnl\n> +ip,nw_src=0.0.0.0/1.0.0.0\n> +ip,nw_src=128.0.0.0/1\n> +ip,nw_src=16.0.0.0/16.0.0.0\n> +ip,nw_src=2.0.0.0/2.0.0.0\n> +ip,nw_src=32.0.0.0/32.0.0.0\n> +ip,nw_src=4.0.0.0/4.0.0.0\n> +ip,nw_src=64.0.0.0/64.0.0.0\n> +ip,nw_src=8.0.0.0/8.0.0.0\n> +])\n> AT_CLEANUP\n>\n> AT_SETUP([ovn -- action parsing])\n> diff --git a/tests/test-ovn.c b/tests/test-ovn.c\n> index 694bc794a923..148ce122d385 100644\n> --- a/tests/test-ovn.c\n> +++ b/tests/test-ovn.c\n> @@ -202,10 +202,12 @@ create_addr_sets(struct shash *addr_sets)\n> static const char *const addrs3[] = {\n> \"00:00:00:00:00:01\", \"00:00:00:00:00:02\", \"00:00:00:00:00:03\",\n> };\n> + static const char *const addrs4[] = {};\n>\n> expr_addr_sets_add(addr_sets, \"set1\", addrs1, 3);\n> expr_addr_sets_add(addr_sets, \"set2\", addrs2, 3);\n> expr_addr_sets_add(addr_sets, \"set3\", addrs3, 3);\n> + expr_addr_sets_add(addr_sets, \"set4\", addrs4, 0);\n> }\n>\n> static bool\n> --\n> 2.10.2\n>\n\nAhh, I missed the case of \"!=\". Thanks Ben for the quick fix!\n\nHan","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"DKqAphAd\"; dkim-atps=neutral"],"Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xqzdQ0p06z9s7m\n\tfor ;\n\tMon, 11 Sep 2017 04:11:05 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id B1C69AB7;\n\tSun, 10 Sep 2017 18:11:02 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 84636A6E\n\tfor ; Sun, 10 Sep 2017 18:11:01 +0000 (UTC)","from mail-vk0-f49.google.com (mail-vk0-f49.google.com\n\t[209.85.213.49])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id 53B5347C\n\tfor ; Sun, 10 Sep 2017 18:11:00 +0000 (UTC)","by mail-vk0-f49.google.com with SMTP id c82so7862274vkd.4\n\tfor ; Sun, 10 Sep 2017 11:11:00 -0700 (PDT)","by 10.176.78.13 with HTTP; Sun, 10 Sep 2017 11:10:58 -0700 (PDT)"],"X-Greylist":"whitelisted by SQLgrey-1.7.6","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=z0lFpcspWOBpXPnVCz1831SkGbllhOtGr2zimqiD0iI=;\n\tb=DKqAphAdE3LScmZRiBlrstZtzw0/aSgThrG5DT9UmeQ6cc7OJBsr/8On6uN38Po8dR\n\truixm9PqhQX1g5jGK4fsa2xgUu1Np+UgMfKEDL1PMwLt+Y1862XgWYTyrZU5Uc/QuOSB\n\t3fhKHyDRmofhpJxg9iDdOaEowepf2e9BEikq/MCyTpRMXK8emg6pT0sL1roLRu+yX/8p\n\thCghiEwUijgQZzDCFfzMyTgE+uFgoQs5+LcR7ljLoQAbEMU586p3EMaIXMGQuMuLRm2s\n\tBtUXx8uM0cG+WD8N5EVzLw0cFQELex5WjGne2lokGpqlKnfSXedRk0EsJPC51CNmyMJT\n\t0cjA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=z0lFpcspWOBpXPnVCz1831SkGbllhOtGr2zimqiD0iI=;\n\tb=RhDfnlxidllaGPVDmi7XR2HY/v0Sv0shnAm3Uv4on1G2eFWR4bRSXjJP49WN25f+NZ\n\tou8T8o4/B4Rppw/fzXm6mrLk4ND1qdXjkBQB7lWzFFr4KcrRfwS+QrcssJ1tkPmUK98e\n\tkUy0dmP2OE3OfujEiG0b3v2lWdK3lcQFyXbxCR0k+adPU1+qgrbIFd944qxoc7yPQa+X\n\t6QjRq6Xj+72BmYGncPxu4WOyPwMudtclAhCNzFAlrL6dCDOUVju6C2f5HnWFEdNyb48Y\n\tQ1oL9crVhMLeMaBcvi9rr6SZoEXsMIZDLq2XLiVU7vdNhgnNhWzT/OXv9isMulgthLW4\n\tudhQ==","X-Gm-Message-State":"AHPjjUhkFjMpVf0xxpVAd2M7jx8tcJLtA9dSXKSRtvTneDyklbCz+3tC\n\tFnnYAndnF/UAHDUkweURGDi2/3GlRw==","X-Google-Smtp-Source":"AOwi7QDX5Iofaixqqjs5Y+/K5oxcJ1xT6fSuQpSQaNYoui1dYtD4Hmk9kWFklZhrhrHkcODwCiQKXrPftTg9wpLSRr0=","X-Received":"by 10.31.63.204 with SMTP id m195mr6411548vka.111.1505067059286; \n\tSun, 10 Sep 2017 11:10:59 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170910175657.GA6175@ovn.org>","References":"<1505023105-90302-1-git-send-email-zhouhan@gmail.com>\n\t<20170910175657.GA6175@ovn.org>","From":"Han Zhou ","Date":"Sun, 10 Sep 2017 11:10:58 -0700","Message-ID":"","To":"Ben Pfaff ","X-Spam-Status":"No, score=0.4 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,\n\tDKIM_VALID_AU, FREEMAIL_FROM, HTML_MESSAGE, NORMAL_HTTP_TO_IP,\n\tRCVD_IN_DNSWL_NONE, \n\tRCVD_IN_SORBS_SPAM autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","X-Content-Filtered-By":"Mailman/MimeDel 2.1.12","Cc":"\"dev@openvswitch.org\" ","Subject":"Re: [ovs-dev] [PATCH] ovn-controller: Fix empty address set parsing\n\tproblem.","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1766357,"web_url":"http://patchwork.ozlabs.org/comment/1766357/","msgid":"<20170911151431.GC6175@ovn.org>","list_archive_url":null,"date":"2017-09-11T15:14:31","subject":"Re: [ovs-dev] [PATCH] ovn-controller: Fix empty address set parsing\n\tproblem.","submitter":{"id":67603,"url":"http://patchwork.ozlabs.org/api/people/67603/","name":"Ben Pfaff","email":"blp@ovn.org"},"content":"On Sun, Sep 10, 2017 at 11:10:58AM -0700, Han Zhou wrote:\n> On Sun, Sep 10, 2017 at 10:56 AM, Ben Pfaff wrote:\n> >\n> > On Sat, Sep 09, 2017 at 10:58:25PM -0700, Han Zhou wrote:\n> > > When an address set is empty, current implementation will generate\n> > > an ovs flow that matches random things (and in most cases matching\n> > > everything) due to a problem in expression parser of constant set.\n> > > This patch fixes it by replacing the expression by a boolean false\n> > > when the set is empty, and adds tests cases accordingly.\n> > >\n> > > Reported-by: Guru Shetty \n> > > Reported-at:\n> https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html\n> > > Signed-off-by: Han Zhou \n> > > ---\n> > > ovn/lib/expr.c | 4 ++++\n> > > tests/ovn.at | 9 +++++++++\n> > > tests/test-ovn.c | 2 ++\n> > > 3 files changed, 15 insertions(+)\n> > >\n> > > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c\n> > > index 060e9ee..6a731de 100644\n> > > --- a/ovn/lib/expr.c\n> > > +++ b/ovn/lib/expr.c\n> > > @@ -612,6 +612,10 @@ make_cmp(struct expr_context *ctx,\n> > > }\n> > > }\n> > >\n> > > + if (!cs->n_values) {\n> > > + e = expr_create_boolean(false);\n> > > + goto exit;\n> > > + }\n> >\n> > Thanks for working on this. I agree with everyone that this is an\n> > important fix.\n> >\n> > I think that it overlooks the behavior of !=. Presumably, == and !=\n> > should yield opposite results. To get that behavior, we want:\n> > e = expr_create_boolean(r == EXPR_R_NE);\n> > instead of:\n> > e = expr_create_boolean(false);\n> >\n> > Here's a v2 that includes this change plus tests for !=. I added myself\n> > as coauthor; I hope that's OK.\n> >\n> > Thanks,\n> >\n> > Ben.\n> >\n> > --8<--------------------------cut here-------------------------->8--\n> >\n> > From: Han Zhou \n> > Date: Sat, 9 Sep 2017 22:58:25 -0700\n> > Subject: [PATCH] ovn-controller: Fix empty address set parsing problem.\n> >\n> > When an address set is empty, current implementation will generate\n> > an ovs flow that matches random things (and in most cases matching\n> > everything) due to a problem in expression parser of constant set.\n> > This patch fixes it by replacing the expression by a boolean false\n> > when the set is empty, and adds tests cases accordingly.\n> >\n> > Reported-by: Guru Shetty \n> > Reported-at:\n> https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html\n> > Signed-off-by: Han Zhou \n> > Co-authored-by: Ben Pfaff \n> > Signed-off-by: Ben Pfaff \n> > ---\n> > ovn/lib/expr.c | 4 ++++\n> > tests/ovn.at | 32 ++++++++++++++++++++++++++++++++\n> > tests/test-ovn.c | 2 ++\n> > 3 files changed, 38 insertions(+)\n> >\n> > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c\n> > index 060e9ee3d088..79ff45762f65 100644\n> > --- a/ovn/lib/expr.c\n> > +++ b/ovn/lib/expr.c\n> > @@ -612,6 +612,10 @@ make_cmp(struct expr_context *ctx,\n> > }\n> > }\n> >\n> > + if (!cs->n_values) {\n> > + e = expr_create_boolean(r == EXPR_R_NE);\n> > + goto exit;\n> > + }\n> > e = make_cmp__(f, r, &cs->values[0]);\n> > for (size_t i = 1; i < cs->n_values; i++) {\n> > e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND,\n> > diff --git a/tests/ovn.at b/tests/ovn.at\n> > index bb9999ce0b1b..f2035290f66e 100644\n> > --- a/tests/ovn.at\n> > +++ b/tests/ovn.at\n> > @@ -623,6 +623,38 @@ dl_src=00:00:00:00:00:02\n> > dl_src=00:00:00:00:00:03\n> > dl_src=ba:be:be:ef:de:ad\n> > ])\n> > +AT_CHECK([expr_to_flow 'ip4.src == {$set4}'], [0], [dnl\n> > +(no flows)\n> > +])\n> > +AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set4}'], [0], [dnl\n> > +ip,nw_src=1.2.3.4\n> > +])\n> > +AT_CHECK([expr_to_flow 'ip4.src == 1.2.3.4 || ip4.src == {$set4}'], [0],\n> [dnl\n> > +ip,nw_src=1.2.3.4\n> > +])\n> > +AT_CHECK([expr_to_flow 'ip4.src != {$set4}'], [0], [dnl\n> > +\n> > +])\n> > +AT_CHECK([expr_to_flow 'ip4.src != {1.0.0.0/8, $set4}'], [0], [dnl\n> > +ip,nw_src=0.0.0.0/1.0.0.0\n> > +ip,nw_src=128.0.0.0/1\n> > +ip,nw_src=16.0.0.0/16.0.0.0\n> > +ip,nw_src=2.0.0.0/2.0.0.0\n> > +ip,nw_src=32.0.0.0/32.0.0.0\n> > +ip,nw_src=4.0.0.0/4.0.0.0\n> > +ip,nw_src=64.0.0.0/64.0.0.0\n> > +ip,nw_src=8.0.0.0/8.0.0.0\n> > +])\n> > +AT_CHECK([expr_to_flow 'ip4.src != 1.0.0.0/8 && ip4.src != {$set4}'],\n> [0], [dnl\n> > +ip,nw_src=0.0.0.0/1.0.0.0\n> > +ip,nw_src=128.0.0.0/1\n> > +ip,nw_src=16.0.0.0/16.0.0.0\n> > +ip,nw_src=2.0.0.0/2.0.0.0\n> > +ip,nw_src=32.0.0.0/32.0.0.0\n> > +ip,nw_src=4.0.0.0/4.0.0.0\n> > +ip,nw_src=64.0.0.0/64.0.0.0\n> > +ip,nw_src=8.0.0.0/8.0.0.0\n> > +])\n> > AT_CLEANUP\n> >\n> > AT_SETUP([ovn -- action parsing])\n> > diff --git a/tests/test-ovn.c b/tests/test-ovn.c\n> > index 694bc794a923..148ce122d385 100644\n> > --- a/tests/test-ovn.c\n> > +++ b/tests/test-ovn.c\n> > @@ -202,10 +202,12 @@ create_addr_sets(struct shash *addr_sets)\n> > static const char *const addrs3[] = {\n> > \"00:00:00:00:00:01\", \"00:00:00:00:00:02\", \"00:00:00:00:00:03\",\n> > };\n> > + static const char *const addrs4[] = {};\n> >\n> > expr_addr_sets_add(addr_sets, \"set1\", addrs1, 3);\n> > expr_addr_sets_add(addr_sets, \"set2\", addrs2, 3);\n> > expr_addr_sets_add(addr_sets, \"set3\", addrs3, 3);\n> > + expr_addr_sets_add(addr_sets, \"set4\", addrs4, 0);\n> > }\n> >\n> > static bool\n> > --\n> > 2.10.2\n> >\n> \n> Ahh, I missed the case of \"!=\". Thanks Ben for the quick fix!\n\nYou're welcome.\n\nI applied this to master, branch-2.8, branch-2.7, and branch-2.6.","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xrWgS2nLYz9s4q\n\tfor ;\n\tTue, 12 Sep 2017 01:14:43 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 7A5D6A81;\n\tMon, 11 Sep 2017 15:14:40 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id A5B71A59\n\tfor ; Mon, 11 Sep 2017 15:14:39 +0000 (UTC)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id 01DC13F9\n\tfor ; Mon, 11 Sep 2017 15:14:38 +0000 (UTC)","from ovn.org (173-228-112-34.dsl.dynamic.fusionbroadband.com\n\t[173.228.112.34]) (Authenticated sender: blp@ovn.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id A302841C0AE;\n\tMon, 11 Sep 2017 17:14:35 +0200 (CEST)"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-Originating-IP":"173.228.112.34","Date":"Mon, 11 Sep 2017 08:14:31 -0700","From":"Ben Pfaff ","To":"Han Zhou ","Message-ID":"<20170911151431.GC6175@ovn.org>","References":"<1505023105-90302-1-git-send-email-zhouhan@gmail.com>\n\t<20170910175657.GA6175@ovn.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.5.23 (2014-03-12)","X-Spam-Status":"No, score=-0.7 required=5.0 tests=RCVD_IN_DNSWL_LOW\n\tautolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Cc":"\"dev@openvswitch.org\" ","Subject":"Re: [ovs-dev] [PATCH] ovn-controller: Fix empty address set parsing\n\tproblem.","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}}]