From 4cce733fc200ebb69fccacc35fde9aa9b7264ad3 Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Wed, 3 Mar 2021 19:38:38 +0300 Subject: [PATCH 1/4] bfdd: require local-address when using multihop If local-address is not supplied, then an incorrect xpath is generated which is not expected by NB CLI. Fixes #7465. Signed-off-by: Igor Ryzhov --- bfdd/bfdd_cli.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/bfdd/bfdd_cli.c b/bfdd/bfdd_cli.c index b9e7903613..84354e243d 100644 --- a/bfdd/bfdd_cli.c +++ b/bfdd/bfdd_cli.c @@ -117,10 +117,14 @@ DEFPY_YANG_NOSH( char source_str[INET6_ADDRSTRLEN + 32]; char xpath[XPATH_MAXLEN], xpath_srcaddr[XPATH_MAXLEN + 32]; - if (multihop) + if (multihop) { + if (!local_address_str) { + vty_out(vty, "%% local-address is required when using multihop\n"); + return CMD_WARNING_CONFIG_FAILED; + } snprintf(source_str, sizeof(source_str), "[source-addr='%s']", local_address_str); - else + } else source_str[0] = 0; slen = snprintf(xpath, sizeof(xpath), From 5e4d0fbc93cac79f1f94c612c64f4ab5949db5da Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Wed, 3 Mar 2021 19:43:00 +0300 Subject: [PATCH 2/4] bfdd: actually return validation error instead of logging Before: ``` (config-bfd)# peer fe80::a00:27ff:fea2:5803 multihop local-address fe80::a00:27ff:fea2:5802 % Configuration failed. Error type: validation ``` After: ``` (config-bfd)# peer fe80::a00:27ff:fea2:5803 multihop local-address fe80::a00:27ff:fea2:5802 % Configuration failed. Error type: validation Error description: When using link-local you must specify an interface ``` Signed-off-by: Igor Ryzhov --- bfdd/bfdd_nb_config.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/bfdd/bfdd_nb_config.c b/bfdd/bfdd_nb_config.c index fe6a0b7905..b8065c6052 100644 --- a/bfdd/bfdd_nb_config.c +++ b/bfdd/bfdd_nb_config.c @@ -55,36 +55,35 @@ static void bfd_session_get_key(bool mhop, const struct lyd_node *dnode, gen_bfd_key(bk, &psa, &lsa, mhop, ifname, vrfname); } -static int bfd_session_create(enum nb_event event, const struct lyd_node *dnode, - union nb_resource *resource, bool mhop) +static int bfd_session_create(struct nb_cb_create_args *args, bool mhop) { struct bfd_session *bs; const char *ifname; struct bfd_key bk; struct prefix p; - switch (event) { + switch (args->event) { case NB_EV_VALIDATE: /* * When `dest-addr` is IPv6 and link-local we must * require interface name, otherwise we can't figure * which interface to use to send the packets. */ - yang_dnode_get_prefix(&p, dnode, "./dest-addr"); + yang_dnode_get_prefix(&p, args->dnode, "./dest-addr"); - ifname = yang_dnode_get_string(dnode, "./interface"); + ifname = yang_dnode_get_string(args->dnode, "./interface"); if (p.family == AF_INET6 && IN6_IS_ADDR_LINKLOCAL(&p.u.prefix6) && strcmp(ifname, "*") == 0) { - zlog_warn( - "%s: when using link-local you must specify an interface.", - __func__); + snprintf( + args->errmsg, args->errmsg_len, + "When using link-local you must specify an interface"); return NB_ERR_VALIDATION; } break; case NB_EV_PREPARE: - bfd_session_get_key(mhop, dnode, &bk); + bfd_session_get_key(mhop, args->dnode, &bk); bs = bfd_key_lookup(bk); /* This session was already configured by another daemon. */ @@ -93,14 +92,14 @@ static int bfd_session_create(enum nb_event event, const struct lyd_node *dnode, SET_FLAG(bs->flags, BFD_SESS_FLAG_CONFIG); bs->refcount++; - resource->ptr = bs; + args->resource->ptr = bs; break; } bs = bfd_session_new(); /* Fill the session key. */ - bfd_session_get_key(mhop, dnode, &bs->key); + bfd_session_get_key(mhop, args->dnode, &bs->key); /* Set configuration flags. */ bs->refcount = 1; @@ -110,23 +109,23 @@ static int bfd_session_create(enum nb_event event, const struct lyd_node *dnode, if (bs->key.family == AF_INET6) SET_FLAG(bs->flags, BFD_SESS_FLAG_IPV6); - resource->ptr = bs; + args->resource->ptr = bs; break; case NB_EV_APPLY: - bs = resource->ptr; + bs = args->resource->ptr; /* Only attempt to registrate if freshly allocated. */ if (bs->discrs.my_discr == 0 && bs_registrate(bs) == NULL) return NB_ERR_RESOURCE; - nb_running_set_entry(dnode, bs); + nb_running_set_entry(args->dnode, bs); break; case NB_EV_ABORT: - bs = resource->ptr; + bs = args->resource->ptr; if (bs->refcount <= 1) - bfd_session_free(resource->ptr); + bfd_session_free(bs); break; } @@ -474,8 +473,7 @@ int bfdd_bfd_profile_desired_echo_transmission_interval_modify( */ int bfdd_bfd_sessions_single_hop_create(struct nb_cb_create_args *args) { - return bfd_session_create(args->event, args->dnode, args->resource, - false); + return bfd_session_create(args, false); } int bfdd_bfd_sessions_single_hop_destroy(struct nb_cb_destroy_args *args) @@ -759,8 +757,7 @@ int bfdd_bfd_sessions_single_hop_desired_echo_transmission_interval_modify( */ int bfdd_bfd_sessions_multi_hop_create(struct nb_cb_create_args *args) { - return bfd_session_create(args->event, args->dnode, args->resource, - true); + return bfd_session_create(args, true); } int bfdd_bfd_sessions_multi_hop_destroy(struct nb_cb_destroy_args *args) From 632f36100c2d36e17446f384e430e8eecfa03d6e Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Wed, 3 Mar 2021 23:10:19 +0300 Subject: [PATCH 3/4] bfdd: forbid creation of the same peer with and without interface name Currently it is possible to configure the same peer with and without interface name: ``` bfd peer 1.1.1.1 ! peer 1.1.1.1 interface enp0s3 ! ``` There are multiple problems with that: 1. Both nodes actually control the same BFD session. So the config is either duplicated or, even worse, different - and there is no way to say which one actually works. 2. When the user deletes both nodes, the session is not actually freed, because its refcount is always greater than 1. Such configuration must be forbidden. User should either have single node with wildcard name or multiple nodes with actual names. Signed-off-by: Igor Ryzhov --- bfdd/bfdd_nb_config.c | 52 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/bfdd/bfdd_nb_config.c b/bfdd/bfdd_nb_config.c index b8065c6052..c8dd5cc3f6 100644 --- a/bfdd/bfdd_nb_config.c +++ b/bfdd/bfdd_nb_config.c @@ -55,10 +55,35 @@ static void bfd_session_get_key(bool mhop, const struct lyd_node *dnode, gen_bfd_key(bk, &psa, &lsa, mhop, ifname, vrfname); } +struct session_iter { + int count; + bool wildcard; +}; + +static int session_iter_cb(const struct lyd_node *dnode, void *arg) +{ + struct session_iter *iter = arg; + const char *ifname; + + ifname = yang_dnode_get_string(dnode, "./interface"); + + if (strmatch(ifname, "*")) + iter->wildcard = true; + + iter->count++; + + return YANG_ITER_CONTINUE; +} + static int bfd_session_create(struct nb_cb_create_args *args, bool mhop) { + const struct lyd_node *sess_dnode; + struct session_iter iter; struct bfd_session *bs; + const char *source; + const char *dest; const char *ifname; + const char *vrfname; struct bfd_key bk; struct prefix p; @@ -80,6 +105,33 @@ static int bfd_session_create(struct nb_cb_create_args *args, bool mhop) "When using link-local you must specify an interface"); return NB_ERR_VALIDATION; } + + iter.count = 0; + iter.wildcard = false; + + sess_dnode = yang_dnode_get_parent(args->dnode, "sessions"); + + dest = yang_dnode_get_string(args->dnode, "./dest-addr"); + vrfname = yang_dnode_get_string(args->dnode, "./vrf"); + + if (mhop) { + source = yang_dnode_get_string(args->dnode, "./source-addr"); + + yang_dnode_iterate(session_iter_cb, &iter, sess_dnode, + "./multi-hop[source-addr='%s'][dest-addr='%s'][vrf='%s']", + source, dest, vrfname); + } else { + yang_dnode_iterate(session_iter_cb, &iter, sess_dnode, + "./single-hop[dest-addr='%s'][vrf='%s']", + dest, vrfname); + } + + if (iter.wildcard && iter.count > 1) { + snprintf( + args->errmsg, args->errmsg_len, + "It is not allowed to configure the same peer with and without ifname"); + return NB_ERR_VALIDATION; + } break; case NB_EV_PREPARE: From 17cb53af259d9916899820196c8da68f40746657 Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Thu, 4 Mar 2021 21:17:20 +0300 Subject: [PATCH 4/4] bfdd: fix echo configuration in profile It's not currently possible to configure echo mode in profile node: ``` (config)# bfd (config-bfd)# profile test (config-bfd-profile)# echo-mode % Echo mode is only available for single hop sessions. (config-bfd-profile)# echo-interval 20 % Echo mode is only available for single hop sessions. ``` Signed-off-by: Igor Ryzhov --- bfdd/bfdd_cli.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/bfdd/bfdd_cli.c b/bfdd/bfdd_cli.c index 84354e243d..5072c76aac 100644 --- a/bfdd/bfdd_cli.c +++ b/bfdd/bfdd_cli.c @@ -57,6 +57,12 @@ bfd_cli_is_single_hop(struct vty *vty) return strstr(VTY_CURR_XPATH, "/single-hop") != NULL; } +static bool +bfd_cli_is_profile(struct vty *vty) +{ + return strstr(VTY_CURR_XPATH, "/bfd/profile") != NULL; +} + /* * Functions. */ @@ -422,7 +428,7 @@ DEFPY_YANG( NO_STR "Configure echo mode\n") { - if (!bfd_cli_is_single_hop(vty)) { + if (!bfd_cli_is_profile(vty) && !bfd_cli_is_single_hop(vty)) { vty_out(vty, "%% Echo mode is only available for single hop sessions.\n"); return CMD_WARNING_CONFIG_FAILED; } @@ -450,7 +456,7 @@ DEFPY_YANG( { char value[32]; - if (!bfd_cli_is_single_hop(vty)) { + if (!bfd_cli_is_profile(vty) && !bfd_cli_is_single_hop(vty)) { vty_out(vty, "%% Echo mode is only available for single hop sessions.\n"); return CMD_WARNING_CONFIG_FAILED; }