diff mbox series

[ovs-dev,v2] ovn-northd: Fix router policy pkt mark over flow if the value is greater than signed int.

Message ID 20200918152917.1481104-1-numans@ovn.org
State Accepted
Headers show
Series [ovs-dev,v2] ovn-northd: Fix router policy pkt mark over flow if the value is greater than signed int. | expand

Commit Message

Numan Siddique Sept. 18, 2020, 3:29 p.m. UTC 
From: Numan Siddique <numans@ovn.org>

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 <aconstan@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
---

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(-)

Comments

Dumitru Ceara Sept. 18, 2020, 3:34 p.m. UTC | #1 
On 9/18/20 5:29 PM, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> 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 <aconstan@redhat.com>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---

Looks good to me.

Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks,
Dumitru
Numan Siddique Sept. 21, 2020, 9:04 a.m. UTC | #2 
On Fri, Sep 18, 2020 at 9:05 PM Dumitru Ceara <dceara@redhat.com> wrote:

> On 9/18/20 5:29 PM, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > 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 <aconstan@redhat.com>
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
>
> Looks good to me.
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>
>

Thanks Dumitru for the review. I applied this patch to master and
branch-20.09.

Numan


>
> Thanks,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
diff mbox series

Patch

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