[ovs-dev] [PATCH ovn] Don't use strcmp for MAC addresses

[Dumitru Ceara]

On 5/8/20 8:51 PM, Mark Michelson wrote:
> strcmp() for MAC addresses will detect that MAC addresses are different
> if they use different capitalization.
> 
> 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.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1829059
> Signed-off-by: Mark Michelson <mmichels at redhat.com>

Hi Mark,

Shouldn't we deal with IPv6 addresses in a case insensitive way too?

For example:
$ ovn-nbctl lr-add lr-test
$ ovn-nbctl --may-exist lrp-add lr-test lrp-test 00:00:00:00:00:01
4242::000a/64
$ ovn-nbctl --may-exist lrp-add lr-test lrp-test 00:00:00:00:00:01
4242::000A/64
ovn-nbctl: lrp-test: port already exists with different network

Apart from that (which can be handled as a separate patch) a few more
minor comments inline.

Thanks,
Dumitru

> ---
>  northd/ovn-northd.c   | 18 +++++++++---------
>  tests/ovn.at          |  9 +++++++++
>  utilities/ovn-nbctl.c | 16 +++++++++-------
>  3 files changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 83e6134b0..df71b1e03 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,10 @@ 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)) {
> +        memset(&svc_monitor_mac_ea, 0, sizeof svc_monitor_mac_ea);

eth_addr_from_string() sets svc_monitor_mac_ea to "eth_addr_zero" if
parsing fails so the memset above should not be needed.

> +        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 +11115,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..bcc869c61 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 192.168.0.0/24

While this is fine for the test purpose, it's a bit weird to see a host
address (.0 of the subnet) used for router port IP.

> +
> +AT_CHECK([ovn-nbctl --may-exist lrp-add r1 rp1 cc:dd:ee:ee:dd:cc 192.168.0.0/24])
> +AT_CLEANUP
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 02fc10c9e..bf8b94734 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -4647,6 +4647,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,7 +4679,9 @@ 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;
> @@ -4715,12 +4723,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;
>