From 0d6d0208a55c786513f472ad690bae1788e173fa Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Thu, 29 Apr 2021 16:31:15 +0300 Subject: [PATCH 1/3] bgpd: fix crash when as/type mismatches in config When we're trying to create the config for already existing router and the AS number or instance type mismatches, we're returning the inconsistency error and don't set the NB entry. Any subsequent command that configures this router will crash because every command relies on the existence of the NB entry. Let's store the entry even when there's a mismatch to prevent the crash. Signed-off-by: Igor Ryzhov --- bgpd/bgp_nb_config.c | 27 ++++++++++++++++++--------- bgpd/bgpd.c | 2 +- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/bgpd/bgp_nb_config.c b/bgpd/bgp_nb_config.c index 307fa4e9bc..1939e78e73 100644 --- a/bgpd/bgp_nb_config.c +++ b/bgpd/bgp_nb_config.c @@ -109,15 +109,24 @@ int bgp_router_create(struct nb_cb_create_args *args) is_new_bgp = (bgp_lookup_by_name(name) == NULL); ret = bgp_get_vty(&bgp, &as, name, inst_type); - switch (ret) { - case BGP_ERR_AS_MISMATCH: - snprintf(args->errmsg, args->errmsg_len, - "BGP instance is already running; AS is %u", - as); - return NB_ERR_INCONSISTENCY; - case BGP_ERR_INSTANCE_MISMATCH: - snprintf(args->errmsg, args->errmsg_len, - "BGP instance type mismatch"); + if (ret) { + switch (ret) { + case BGP_ERR_AS_MISMATCH: + snprintf( + args->errmsg, args->errmsg_len, + "BGP instance is already running; AS is %u", + as); + break; + case BGP_ERR_INSTANCE_MISMATCH: + snprintf(args->errmsg, args->errmsg_len, + "BGP instance type mismatch"); + break; + } + + UNSET_FLAG(bgp->vrf_flags, BGP_VRF_AUTO); + + nb_running_set_entry(args->dnode, bgp); + return NB_ERR_INCONSISTENCY; } diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 20bb5e5320..dcc29f4188 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -3374,13 +3374,13 @@ int bgp_lookup_by_as_name_type(struct bgp **bgp_val, as_t *as, const char *name, bgp = bgp_get_default(); if (bgp) { + *bgp_val = bgp; if (bgp->as != *as) { *as = bgp->as; return BGP_ERR_AS_MISMATCH; } if (bgp->inst_type != inst_type) return BGP_ERR_INSTANCE_MISMATCH; - *bgp_val = bgp; return BGP_SUCCESS; } *bgp_val = NULL; From debb7b1931cdf5b3e06ca244bf09bb009b5a8677 Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Mon, 26 Apr 2021 15:43:58 +0300 Subject: [PATCH 2/3] bgpd: simplify bgp_global_local_as_modify code No need for complicated code to check instance existence. Signed-off-by: Igor Ryzhov --- bgpd/bgp_nb_config.c | 58 ++++++++------------------------------------ 1 file changed, 10 insertions(+), 48 deletions(-) diff --git a/bgpd/bgp_nb_config.c b/bgpd/bgp_nb_config.c index 1939e78e73..18a4ec9053 100644 --- a/bgpd/bgp_nb_config.c +++ b/bgpd/bgp_nb_config.c @@ -228,64 +228,26 @@ int bgp_router_destroy(struct nb_cb_destroy_args *args) int bgp_global_local_as_modify(struct nb_cb_modify_args *args) { struct bgp *bgp; - as_t as; - const struct lyd_node *vrf_dnode; - const char *vrf_name; - const char *name = NULL; - enum bgp_instance_type inst_type; - int ret; - bool is_view_inst = false; switch (args->event) { case NB_EV_VALIDATE: - as = yang_dnode_get_uint32(args->dnode, NULL); - - inst_type = BGP_INSTANCE_TYPE_DEFAULT; - - vrf_dnode = yang_dnode_get_parent(args->dnode, - "control-plane-protocol"); - vrf_name = yang_dnode_get_string(vrf_dnode, "./vrf"); - - if (strmatch(vrf_name, VRF_DEFAULT_NAME)) { - name = NULL; - } else { - name = vrf_name; - inst_type = BGP_INSTANCE_TYPE_VRF; - } - - is_view_inst = yang_dnode_get_bool(args->dnode, - "../instance-type-view"); - if (is_view_inst) - inst_type = BGP_INSTANCE_TYPE_VIEW; - - ret = bgp_lookup_by_as_name_type(&bgp, &as, name, inst_type); - switch (ret) { - case BGP_ERR_AS_MISMATCH: + /* + * Changing AS number is not allowed, but we must allow it + * once, when the BGP instance is created the first time. + * If the instance already exists - return the validation + * error. + */ + bgp = nb_running_get_entry_non_rec(args->dnode->parent->parent, + NULL, false); + if (bgp) { snprintf(args->errmsg, args->errmsg_len, - "BGP instance is already running; AS is %u", - as); - return NB_ERR_VALIDATION; - case BGP_ERR_INSTANCE_MISMATCH: - snprintf(args->errmsg, args->errmsg_len, - "BGP instance type mismatch"); + "Changing AS number is not allowed"); return NB_ERR_VALIDATION; } break; case NB_EV_PREPARE: case NB_EV_ABORT: - return NB_OK; case NB_EV_APPLY: - /* NOTE: handled in bgp_global_create callback, the as change - * will be rejected in validate phase. - */ - as = yang_dnode_get_uint32(args->dnode, NULL); - bgp = nb_running_get_entry(args->dnode, NULL, true); - if (bgp->as != as) { - snprintf(args->errmsg, args->errmsg_len, - "BGP instance is already running; AS is %u", - bgp->as); - return NB_ERR_INCONSISTENCY; - } break; } From d3e3677096e5cf30116ca63879caf44e25b080ad Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Mon, 26 Apr 2021 15:46:58 +0300 Subject: [PATCH 3/3] bgpd: forbid modification of bgp instance type If a user issues the following commands: ``` router bgp 65000 vrf red router bgp 65000 view red ``` bgpd ends up having NB config inconsistent with actual data. Signed-off-by: Igor Ryzhov --- bgpd/bgp_nb_config.c | 17 ++++++++++++++++- bgpd/bgp_vty.c | 4 ++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_nb_config.c b/bgpd/bgp_nb_config.c index 18a4ec9053..e753d87a38 100644 --- a/bgpd/bgp_nb_config.c +++ b/bgpd/bgp_nb_config.c @@ -1483,12 +1483,27 @@ int bgp_global_global_config_timers_keepalive_modify( */ int bgp_global_instance_type_view_modify(struct nb_cb_modify_args *args) { + struct bgp *bgp; + switch (args->event) { case NB_EV_VALIDATE: + /* + * Changing instance type is not allowed, but we must allow it + * once, when the BGP instance is created the first time. + * If the instance already exists - return the validation + * error. + */ + bgp = nb_running_get_entry_non_rec(args->dnode->parent->parent, + NULL, false); + if (bgp) { + snprintf(args->errmsg, args->errmsg_len, + "Changing instance type is not allowed"); + return NB_ERR_VALIDATION; + } + break; case NB_EV_PREPARE: case NB_EV_ABORT: case NB_EV_APPLY: - /* TODO: implement me. */ break; } diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 1e465d2620..d82c929154 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -1388,6 +1388,10 @@ DEFUN_YANG_NOSH(router_bgp, nb_cli_enqueue_change(vty, "./global/instance-type-view", NB_OP_MODIFY, "true"); + } else { + nb_cli_enqueue_change(vty, + "./global/instance-type-view", + NB_OP_MODIFY, "false"); } ret = nb_cli_apply_changes(vty, base_xpath);