| Message ID | 20200508185151.458467-1-mmichels@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,ovn] Don't use strcmp for MAC addresses | expand |
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@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; >
On 5/11/20 5:33 AM, Dumitru Ceara wrote: > 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@redhat.com> > > Hi Mark, > > Shouldn't we deal with IPv6 addresses in a case insensitive way too? Yes, we should. IPv6 has a double-barreled problem. First, there's the case sensitivity issue. Second, there's the fact that the same address can be expressed multiple ways. 4242::000a == 4242:0000:0000:0000:0000:0000:0000:000a == 4242:0::000a, etc. String comparisons of IPv6 addresses are a real bad idea. One example where it is done correctly is in the load balancing VIP code. So if we find places where it is done wrong, the load balancer code provides a sample for how to fix it. > > 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; >> >
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); + 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 + +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;
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@redhat.com> --- northd/ovn-northd.c | 18 +++++++++--------- tests/ovn.at | 9 +++++++++ utilities/ovn-nbctl.c | 16 +++++++++------- 3 files changed, 27 insertions(+), 16 deletions(-)