[ovs-dev] [PATCH ovn v2] Avoid case sensitive comparisons for MAC and IPv6 addresses
Dumitru Ceara
dceara at redhat.com
Thu May 14 08:42:27 UTC 2020
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 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;
>
More information about the dev
mailing list