diff mbox series

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

Message ID 20200513193930.3361135-1-mmichels@redhat.com
State New
Headers show
Series [ovs-dev,ovn,v2] Avoid case sensitive comparisons for MAC and IPv6 addresses | expand

Commit Message

Mark Michelson May 13, 2020, 7:39 p.m. UTC
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@redhat.com>
---
 northd/ovn-northd.c   | 17 ++++++------
 tests/ovn.at          |  9 +++++++
 utilities/ovn-nbctl.c | 60 +++++++++++++++++++++++++++++++------------
 3 files changed, 61 insertions(+), 25 deletions(-)

Comments

0-day Robot May 13, 2020, 8:16 p.m. UTC | #1
Bleep bloop.  Greetings Mark Michelson, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 83 characters long (recommended limit is 79)
#158 FILE: utilities/ovn-nbctl.c:4733:
        struct sset *orig_networks = lrp_network_sset((const char **)lrp->networks,

Lines checked: 197, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Dumitru Ceara May 14, 2020, 8:42 a.m. UTC | #2
On 5/13/20 9:39 PM, Mark Michelson wrote:
> 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.
> 

Hi Mark,

I'm not sure if we should handle everything in this single patch but
there's also the case of NAT that doesn't work properly wrt. string
comparisons:

$ ovn-nbctl lr-nat-add lr0 dnat_and_snat 4200:AB:00::AB:1
4300:AB:00::AB:1 p1 00:00:00:00:0a:01

$ ovn-nbctl lr-nat-add lr0 dnat_and_snat 4200:ab:00::AB:1
4300:AB:00::AB:1 p1

$ ovn-nbctl lr-nat-add lr0 dnat_and_snat 4200:ab:00::Ab:1
4300:ab:00::AB:1 p1 00:00:00:00:0A:01

ovn-nbctl lr-nat-list lr0
TYPE             EXTERNAL_IP        EXTERNAL_PORT    LOGICAL_IP
   EXTERNAL_MAC         LOGICAL_PORT
dnat_and_snat    4200:AB:00::AB:                     4300:AB:00::AB:1
   00:00:00:00:0a:01    p1
dnat_and_snat    4200:ab:00::AB:                     4300:AB:00::AB:1
   00:00:00:00:0a:01    p1
dnat_and_snat    4200:ab:00::Ab:                     4300:ab:00::AB:1
   00:00:00:00:0A:01    p1

Regards,
Dumitru

> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1829059
> Signed-off-by: Mark Michelson <mmichels@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;
>
Mark Michelson May 14, 2020, 12:29 p.m. UTC | #3
On 5/14/20 4:42 AM, Dumitru Ceara wrote:
> On 5/13/20 9:39 PM, Mark Michelson wrote:
>> 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.
>>
> 
> Hi Mark,
> 
> I'm not sure if we should handle everything in this single patch but
> there's also the case of NAT that doesn't work properly wrt. string
> comparisons:
> 
> $ ovn-nbctl lr-nat-add lr0 dnat_and_snat 4200:AB:00::AB:1
> 4300:AB:00::AB:1 p1 00:00:00:00:0a:01
> 
> $ ovn-nbctl lr-nat-add lr0 dnat_and_snat 4200:ab:00::AB:1
> 4300:AB:00::AB:1 p1
> 
> $ ovn-nbctl lr-nat-add lr0 dnat_and_snat 4200:ab:00::Ab:1
> 4300:ab:00::AB:1 p1 00:00:00:00:0A:01
> 
> ovn-nbctl lr-nat-list lr0
> TYPE             EXTERNAL_IP        EXTERNAL_PORT    LOGICAL_IP
>     EXTERNAL_MAC         LOGICAL_PORT
> dnat_and_snat    4200:AB:00::AB:                     4300:AB:00::AB:1
>     00:00:00:00:0a:01    p1
> dnat_and_snat    4200:ab:00::AB:                     4300:AB:00::AB:1
>     00:00:00:00:0a:01    p1
> dnat_and_snat    4200:ab:00::Ab:                     4300:ab:00::AB:1
>     00:00:00:00:0A:01    p1
> 
> Regards,
> Dumitru

Hi Dumitru,

You've found a sneaky one. nbctl_lr_nat_add() attempts to get around 
this by  normalizing the IP string of the new NAT IPs. However, it 
doesn't also normalize the IP string of the existing NAT IPs.

> 
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1829059
>> Signed-off-by: Mark Michelson <mmichels@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;
>>
>
diff mbox series

Patch

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;