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

numans at ovn.org numans at ovn.org
Thu Sep 17 15:49:15 UTC 2020


From: Numan Siddique <numans at 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 using str_to_uint().

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 at redhat.com>
Signed-off-by: Numan Siddique <numans at ovn.org>
---
 northd/ovn-northd.c | 16 ++++++++++++++--
 tests/ovn.at        | 37 +++++++++++++++++++++++++++++++------
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index cfec6a2c86..df2e39450c 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -7523,6 +7523,18 @@ get_outport_for_routing_policy_nexthop(struct ovn_datapath *od,
     return NULL;
 }
 
+static uint32_t
+get_lr_policy_pkt_mark(const struct nbrec_logical_router_policy *rule)
+{
+    const char *str_pkt_mark = smap_get(&rule->options, "pkt_mark");
+    uint32_t pkt_mark = 0;
+    if (str_pkt_mark) {
+        str_to_uint(str_pkt_mark, 10, &pkt_mark);
+    }
+
+    return pkt_mark;
+}
+
 static void
 build_routing_policy_flow(struct hmap *lflows, struct ovn_datapath *od,
                           struct hmap *ports,
@@ -7547,7 +7559,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 = get_lr_policy_pkt_mark(rule);
         if (pkt_mark) {
             ds_put_format(&actions, "pkt.mark = %u; ", pkt_mark);
         }
@@ -7568,7 +7580,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 = get_lr_policy_pkt_mark(rule);
         if (pkt_mark) {
             ds_put_format(&actions, "pkt.mark = %u; ", pkt_mark);
         }
diff --git a/tests/ovn.at b/tests/ovn.at
index a6f1fb58f8..753b9079fb 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,33 @@ 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_CLEANUP([hv1])
 AT_CLEANUP
 
-- 
2.26.2



More information about the dev mailing list