From patchwork Fri Sep 18 15:29:17 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1367018 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.138; helo=whitealder.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4BtHnZ6BQ4z9sTC for ; Sat, 19 Sep 2020 01:29:38 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 25599873F2; Fri, 18 Sep 2020 15:29:37 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 9S4PPKW1ACy3; Fri, 18 Sep 2020 15:29:34 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by whitealder.osuosl.org (Postfix) with ESMTP id 68FEC86A63; Fri, 18 Sep 2020 15:29:34 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 5B71EC0859; Fri, 18 Sep 2020 15:29:34 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id F1D2FC0051 for ; Fri, 18 Sep 2020 15:29:32 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id E0AB986CF9 for ; Fri, 18 Sep 2020 15:29:32 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 8D8lTcYa6isd for ; Fri, 18 Sep 2020 15:29:31 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay10.mail.gandi.net (relay10.mail.gandi.net [217.70.178.230]) by whitealder.osuosl.org (Postfix) with ESMTPS id 4DF5586A63 for ; Fri, 18 Sep 2020 15:29:31 +0000 (UTC) Received: from nusiddiq.home.org.com (unknown [115.99.187.142]) (Authenticated sender: numans@ovn.org) by relay10.mail.gandi.net (Postfix) with ESMTPSA id 621B0240018; Fri, 18 Sep 2020 15:29:25 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Fri, 18 Sep 2020 20:59:17 +0530 Message-Id: <20200918152917.1481104-1-numans@ovn.org> X-Mailer: git-send-email 2.26.2 MIME-Version: 1.0 Cc: Alexander Constantinescu Subject: [ovs-dev] [PATCH ovn v2] ovn-northd: Fix router policy pkt mark over flow if the value is greater than signed int. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" From: Numan Siddique If the value of pkt_mark in the router policy options is greater than 2147483647, we are ignoring it. This is because we use smap_get_int(). This patch fixes this issue by adding a wrapper function - ovn_smap_get_uint(). The OpenvSwitch library doesn't have this smap helper function. ovn_smap_get_uint() needs to be removed once the OpenvSwitch library has this helper function. A comment has been added in the code for this. Fixes: a123ef0fb8fd("Support packet metadata marking for logical router policies.") Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1878248 Reported-by: Alexander Constantinescu Signed-off-by: Numan Siddique Acked-by: Dumitru Ceara --- v1 -> v2 -------- * Added a wrapper function ovn_smap_get_uint() in lib/util.c lib/ovn-util.c | 20 ++++++++++++++++++++ lib/ovn-util.h | 5 +++++ northd/ovn-northd.c | 4 ++-- tests/ovn.at | 43 +++++++++++++++++++++++++++++++++++++------ 4 files changed, 64 insertions(+), 8 deletions(-) diff --git a/lib/ovn-util.c b/lib/ovn-util.c index cdb5e18fb0..47c7f57a0e 100644 --- a/lib/ovn-util.c +++ b/lib/ovn-util.c @@ -641,3 +641,23 @@ str_tolower(const char *orig) return copy; } + +/* This is a wrapper function which get the value associated with 'key' in + * 'smap' and converts it to an unsigned int. If 'key' is not in 'smap' or a + * valid unsigned integer can't be parsed from it's value, returns 'def'. + * + * Note: Remove this function once OpenvSwitch library (lib/smap.h) has this + * helper function. + */ +unsigned int +ovn_smap_get_uint(const struct smap *smap, const char *key, unsigned int def) +{ + const char *value = smap_get(smap, key); + unsigned int u_value; + + if (!value || !str_to_uint(value, 10, &u_value)) { + return def; + } + + return u_value; +} diff --git a/lib/ovn-util.h b/lib/ovn-util.h index d9aadcbc03..a597efb505 100644 --- a/lib/ovn-util.h +++ b/lib/ovn-util.h @@ -32,6 +32,7 @@ struct eth_addr; struct sbrec_port_binding; struct sbrec_datapath_binding; struct unixctl_conn; +struct smap; struct ipv4_netaddr { ovs_be32 addr; /* 192.168.10.123 */ @@ -152,6 +153,10 @@ char *normalize_ipv4_prefix(ovs_be32 ipv4, unsigned int plen); char *normalize_ipv6_prefix(struct in6_addr ipv6, unsigned int plen); char *normalize_v46_prefix(const struct v46_ip *prefix, unsigned int plen); +/* Temporary util function until ovs library has smap_get_unit. */ +unsigned int ovn_smap_get_uint(const struct smap *smap, const char *key, + unsigned int def); + /* Returns a lowercase copy of orig. * Caller must free the returned string. */ diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index cfec6a2c86..0c12a7437e 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -7547,7 +7547,7 @@ build_routing_policy_flow(struct hmap *lflows, struct ovn_datapath *od, rule->priority, rule->nexthop); return; } - uint32_t pkt_mark = smap_get_int(&rule->options, "pkt_mark", 0); + uint32_t pkt_mark = ovn_smap_get_uint(&rule->options, "pkt_mark", 0); if (pkt_mark) { ds_put_format(&actions, "pkt.mark = %u; ", pkt_mark); } @@ -7568,7 +7568,7 @@ build_routing_policy_flow(struct hmap *lflows, struct ovn_datapath *od, } else if (!strcmp(rule->action, "drop")) { ds_put_cstr(&actions, "drop;"); } else if (!strcmp(rule->action, "allow")) { - uint32_t pkt_mark = smap_get_int(&rule->options, "pkt_mark", 0); + uint32_t pkt_mark = ovn_smap_get_uint(&rule->options, "pkt_mark", 0); if (pkt_mark) { ds_put_format(&actions, "pkt.mark = %u; ", pkt_mark); } diff --git a/tests/ovn.at b/tests/ovn.at index a6f1fb58f8..b2876b806e 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -20589,7 +20589,7 @@ pol4=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=2001 pol5=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=1001) ovn-nbctl set logical_router_policy $pol4 options:pkt_mark=4 -ovn-nbctl set logical_router_policy $pol5 options:pkt_mark=5 +ovn-nbctl set logical_router_policy $pol5 options:pkt_mark=4294967295 ovn-nbctl --wait=hv sync OVS_WAIT_UNTIL([ @@ -20607,9 +20607,11 @@ OVS_WAIT_UNTIL([ grep "load:0x4->NXM_NX_PKT_MARK" -c) ]) +as hv1 ovs-ofctl dump-flows br-int table=20 | grep NXM_NX_PKT_MARK + OVS_WAIT_UNTIL([ test 1 -eq $(as hv1 ovs-ofctl dump-flows br-int table=20 | \ - grep "load:0x5->NXM_NX_PKT_MARK" -c) + grep "load:0xffffffff->NXM_NX_PKT_MARK" -c) ]) AT_CHECK([as hv1 ovs-ofctl del-flows br-phys]) @@ -20620,7 +20622,7 @@ table=0, priority=100, pkt_mark=0x64 actions=drop table=0, priority=100, pkt_mark=0x2 actions=drop table=0, priority=100, pkt_mark=0x3 actions=drop table=0, priority=100, pkt_mark=0x4 actions=drop -table=0, priority=100, pkt_mark=0x5 actions=drop +table=0, priority=100, pkt_mark=0xffffffff actions=drop ]) AT_CHECK([as hv1 ovs-ofctl --protocols=OpenFlow13 add-flows br-phys flows.txt]) @@ -20690,7 +20692,7 @@ AT_CHECK([ AT_CHECK([ test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \ - grep "priority=100,pkt_mark=0x5" | \ + grep "priority=100,pkt_mark=0xffffffff" | \ grep "n_packets=0" -c) ]) @@ -20745,7 +20747,7 @@ send_icmp6_packet hv1 hv1-vif2 505400000004 00000000ff01 ${src_ip6} ${dst_ip6} OVS_WAIT_UNTIL([ test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \ - grep "priority=100,pkt_mark=0x5" | \ + grep "priority=100,pkt_mark=0xffffffff" | \ grep "n_packets=1" -c) ]) @@ -20771,10 +20773,39 @@ OVS_WAIT_UNTIL([ AT_CHECK([ test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \ - grep "priority=100,pkt_mark=0x5" | \ + grep "priority=100,pkt_mark=0xffffffff" | \ grep "n_packets=1" -c) ]) +AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_policy.*priority=1001" | sort], [0], [dnl + table=12(lr_in_policy ), priority=1001 , dnl +match=(ip6), action=(pkt.mark = 4294967295; next;) +]) + +ovn-nbctl --wait=hv set logical_router_policy $pol5 options:pkt_mark=-1 +AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_policy.*priority=1001" | sort], [0], [dnl + table=12(lr_in_policy ), priority=1001 , dnl +match=(ip6), action=(next;) +]) + +ovn-nbctl --wait=hv set logical_router_policy $pol5 options:pkt_mark=2147483648 +AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_policy.*priority=1001" | sort], [0], [dnl + table=12(lr_in_policy ), priority=1001 , dnl +match=(ip6), action=(pkt.mark = 2147483648; next;) +]) + +ovn-nbctl --wait=hv set logical_router_policy $pol5 options:pkt_mark=foo +AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_policy.*priority=1001" | sort], [0], [dnl + table=12(lr_in_policy ), priority=1001 , dnl +match=(ip6), action=(next;) +]) + +ovn-nbctl --wait=hv set logical_router_policy $pol5 options:pkt_mark=4294967296 +AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_policy.*priority=1001" | sort], [0], [dnl + table=12(lr_in_policy ), priority=1001 , dnl +match=(ip6), action=(next;) +]) + OVN_CLEANUP([hv1]) AT_CLEANUP