[ovs-dev] [PATCH ovn v2] Avoid case sensitive comparisons for MAC and IPv6 addresses

Mark Michelson mmichels at redhat.com
Wed May 13 19:39:30 UTC 2020


MAC addresses and IPv6 addresses use hex digits which are appropriate to
express with either uppercase or lowercase letters. strcmp() detects two
MAC or IPv6 addresses as different if theier capitalization differs.

This fixes the issue by converting MAC strings to struct eth_addr and
comparing those instead. This specifically is done when MAC addresses
are provided via user-input. For cases where the MAC strings are not
user-generated (e.g. pinctrl put_mac_binding) the code has not been
touched.

For IPv6 addresses, we use a normalized string format to compare them
instead of using the raw user input or what is stored in the database.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1829059
Signed-off-by: Mark Michelson <mmichels at redhat.com>
---
 northd/ovn-northd.c   | 17 ++++++------
 tests/ovn.at          |  9 +++++++
 utilities/ovn-nbctl.c | 60 +++++++++++++++++++++++++++++++------------
 3 files changed, 61 insertions(+), 25 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 83e6134b0..5cd60dcd1 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -91,6 +91,7 @@ static bool controller_event_en;
  * defined in Service_Monitor Southbound table. Since these packets
  * all locally handled, having just one mac is good enough. */
 static char svc_monitor_mac[ETH_ADDR_STRLEN + 1];
+static struct eth_addr svc_monitor_mac_ea;
 
 /* Default probe interval for NB and SB DB connections. */
 #define DEFAULT_PROBE_INTERVAL_MSEC 5000
@@ -3314,8 +3315,10 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs,
                 ovs_assert(mon_info);
                 sbrec_service_monitor_set_options(
                     mon_info->sbrec_mon, &lb_health_check->options);
+                struct eth_addr ea;
                 if (!mon_info->sbrec_mon->src_mac ||
-                    strcmp(mon_info->sbrec_mon->src_mac, svc_monitor_mac)) {
+                    !eth_addr_from_string(mon_info->sbrec_mon->src_mac, &ea) ||
+                    !eth_addr_equals(ea, svc_monitor_mac_ea)) {
                     sbrec_service_monitor_set_src_mac(mon_info->sbrec_mon,
                                                       svc_monitor_mac);
                 }
@@ -11088,12 +11091,9 @@ ovnnb_db_run(struct northd_context *ctx,
 
     const char *monitor_mac = smap_get(&nb->options, "svc_monitor_mac");
     if (monitor_mac) {
-        struct eth_addr addr;
-
-        memset(&addr, 0, sizeof addr);
-        if (eth_addr_from_string(monitor_mac, &addr)) {
+        if (eth_addr_from_string(monitor_mac, &svc_monitor_mac_ea)) {
             snprintf(svc_monitor_mac, sizeof svc_monitor_mac,
-                     ETH_ADDR_FMT, ETH_ADDR_ARGS(addr));
+                     ETH_ADDR_FMT, ETH_ADDR_ARGS(svc_monitor_mac_ea));
         } else {
             monitor_mac = NULL;
         }
@@ -11114,10 +11114,9 @@ ovnnb_db_run(struct northd_context *ctx,
         }
 
         if (!monitor_mac) {
-            struct eth_addr addr;
-            eth_addr_random(&addr);
+            eth_addr_random(&svc_monitor_mac_ea);
             snprintf(svc_monitor_mac, sizeof svc_monitor_mac,
-                     ETH_ADDR_FMT, ETH_ADDR_ARGS(addr));
+                     ETH_ADDR_FMT, ETH_ADDR_ARGS(svc_monitor_mac_ea));
             smap_replace(&options, "svc_monitor_mac", svc_monitor_mac);
         }
 
diff --git a/tests/ovn.at b/tests/ovn.at
index 52d994972..1676d8c04 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -19179,3 +19179,12 @@ OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [expected])
 
 OVN_CLEANUP([hv1])
 AT_CLEANUP
+
+AT_SETUP([ovn -- Case-insensitive MAC])
+ovn_start
+
+ovn-nbctl lr-add r1
+ovn-nbctl lrp-add r1 rp1 CC:DD:EE:EE:DD:CC AEF0::1/64 BEF0::1/64
+
+AT_CHECK([ovn-nbctl --may-exist lrp-add r1 rp1 cc:dd:ee:ee:dd:cc aef0::1/64 bef0:0000:0000:0000::1/64])
+AT_CLEANUP
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index 02fc10c9e..95bf7f5fd 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -4614,6 +4614,23 @@ nbctl_lrp_get_gateway_chassis(struct ctl_context *ctx)
     free(gcs);
 }
 
+static struct sset *
+lrp_network_sset(const char **networks, int n_networks)
+{
+    struct sset *network_set = xzalloc(sizeof *network_set);
+    sset_init(network_set);
+    for (int i = 0; i < n_networks; i++) {
+        char *norm = normalize_prefix_str(networks[i]);
+        if (!norm) {
+            sset_destroy(network_set);
+            free(network_set);
+            return NULL;
+        }
+        sset_add_and_free(network_set, norm);
+    }
+    return network_set;
+}
+
 static void
 nbctl_lrp_add(struct ctl_context *ctx)
 {
@@ -4647,6 +4664,12 @@ nbctl_lrp_add(struct ctl_context *ctx)
     char **settings = (char **) &ctx->argv[n_networks + 4];
     int n_settings = ctx->argc - 4 - n_networks;
 
+    struct eth_addr ea;
+    if (!eth_addr_from_string(mac, &ea)) {
+        ctl_error(ctx, "%s: invalid mac address %s", lrp_name, mac);
+        return;
+    }
+
     const struct nbrec_logical_router_port *lrp;
     error = lrp_by_name_or_uuid(ctx, lrp_name, false, &lrp);
     if (error) {
@@ -4673,23 +4696,34 @@ nbctl_lrp_add(struct ctl_context *ctx)
             return;
         }
 
-        if (strcmp(mac, lrp->mac)) {
+        struct eth_addr lrp_ea;
+        eth_addr_from_string(lrp->mac, &lrp_ea);
+        if (!eth_addr_equals(ea, lrp_ea)) {
             ctl_error(ctx, "%s: port already exists with mac %s", lrp_name,
                       lrp->mac);
             return;
         }
 
-        struct sset new_networks = SSET_INITIALIZER(&new_networks);
-        for (int i = 0; i < n_networks; i++) {
-            sset_add(&new_networks, networks[i]);
+        struct sset *new_networks = lrp_network_sset(networks, n_networks);
+        if (!new_networks) {
+            ctl_error(ctx, "%s: Invalid networks configured", lrp_name);
+            return;
+        }
+        struct sset *orig_networks = lrp_network_sset((const char **)lrp->networks,
+                                                      lrp->n_networks);
+        if (!orig_networks) {
+            ctl_error(ctx, "%s: Existing port has invalid networks configured",
+                      lrp_name);
+            sset_destroy(new_networks);
+            free(new_networks);
+            return;
         }
 
-        struct sset orig_networks = SSET_INITIALIZER(&orig_networks);
-        sset_add_array(&orig_networks, lrp->networks, lrp->n_networks);
-
-        bool same_networks = sset_equals(&orig_networks, &new_networks);
-        sset_destroy(&orig_networks);
-        sset_destroy(&new_networks);
+        bool same_networks = sset_equals(orig_networks, new_networks);
+        sset_destroy(orig_networks);
+        free(orig_networks);
+        sset_destroy(new_networks);
+        free(new_networks);
         if (!same_networks) {
             ctl_error(ctx, "%s: port already exists with different network",
                       lrp_name);
@@ -4715,12 +4749,6 @@ nbctl_lrp_add(struct ctl_context *ctx)
         return;
     }
 
-    struct eth_addr ea;
-    if (!eth_addr_from_string(mac, &ea)) {
-        ctl_error(ctx, "%s: invalid mac address %s", lrp_name, mac);
-        return;
-    }
-
     for (int i = 0; i < n_networks; i++) {
         ovs_be32 ipv4;
         unsigned int plen;
-- 
2.25.4



More information about the dev mailing list