From 4d0e7a49cf8d4311a485281fa50bbff6ee8ca6cc Mon Sep 17 00:00:00 2001 From: Don Slice Date: Fri, 16 Jul 2021 14:36:10 -0400 Subject: [PATCH 1/6] bgpd: VRF-Lite fix default bgp delete 1. bgp coredump is observed when we delete default bgp instance when we have multi-vrf; and route-leaking is enabled between default, non-default vrfs. Removing default router bgp when routes leaked between non-default vrfs. - Routes are leaked from VRF-A to VRF-B - VPN table is created with auto RD/RT in default instance. - Default instance is deleted, we try to unimport the routes from all VRFs - non-default VRF schedules a work-queue to process deleted routes. - Meanwhile default bgp instance clears VPN tables and free the route entries as well, which are still referenced by non-default VRFs which have imported routes. - When work queue process starts to delete imported route in VRF-A it cores as it accesses freed memory. - Whenever we delete bgp in default vrf, we skip deleting routes in the vpn table, import and export lists. - The default hidden bgp instance will not be listed in any of the show commands. - Whenever we create new default instance, handle it with AS number change i.e. old hidden default bgp's AS number is updated and also changing local_as for all peers. 2. A default instance is created with ASN of the vrf with the import statement. This may not be the ASN desired for the default table - First problem with current behavior. Define two vrfs with different ASNs and then add import between. starting without any bgp config (no default instance) A default instance is created with ASN of the vrf with the import statement. This may not be the ASN desired for the default table - Second related problem. Start with a default instance and a vrf in a different ASN. Do an import statement in the vrf for a bgp vrf instance not yet defined and it auto-creates that bgp/vrf instance and it inherits the ASN of the importing vrf - Handle bgp instances with different ASNs and handle ASN for auto created BGP instance Signed-off-by: Kantesh Mundaragi --- bgpd/bgp_damp.c | 3 +- bgpd/bgp_mplsvpn.c | 3 +- bgpd/bgp_route.c | 46 ++++++++++----- bgpd/bgp_vty.c | 61 ++++++++++++++++---- bgpd/bgpd.c | 137 +++++++++++++++++++++++++++++++++------------ bgpd/bgpd.h | 17 ++++++ 6 files changed, 204 insertions(+), 63 deletions(-) diff --git a/bgpd/bgp_damp.c b/bgpd/bgp_damp.c index 339bfae56d..93f5a19902 100644 --- a/bgpd/bgp_damp.c +++ b/bgpd/bgp_damp.c @@ -779,7 +779,8 @@ int bgp_show_dampening_parameters(struct vty *vty, afi_t afi, safi_t safi, bool use_json = CHECK_FLAG(show_flags, BGP_SHOW_OPT_JSON); bgp = bgp_get_default(); - if (bgp == NULL) { + + if (bgp == NULL || IS_BGP_INSTANCE_HIDDEN(bgp)) { vty_out(vty, "No BGP process is configured\n"); return CMD_WARNING; } diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index f5dbb4aa58..9db73a1bd1 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -3866,7 +3866,8 @@ void bgp_vpn_leak_unimport(struct bgp *from_bgp) bool is_vrf_leak_bind; int debug; - if (from_bgp->inst_type != BGP_INSTANCE_TYPE_VRF) + if (from_bgp->inst_type != BGP_INSTANCE_TYPE_VRF && + from_bgp->inst_type != BGP_INSTANCE_TYPE_DEFAULT) return; debug = (BGP_DEBUG(vpn, VPN_LEAK_TO_VRF) | diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 1f1b900444..223e0cb88a 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -3618,7 +3618,16 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest, struct bgp_path_info_pair old_and_new; int debug = 0; - if (CHECK_FLAG(bgp->flags, BGP_FLAG_DELETE_IN_PROGRESS)) { + /* + * For default bgp instance, which is deleted i.e. marked hidden + * we are skipping SAFI_MPLS_VPN route table deletion + * in bgp_cleanup_routes. + * So, we need to delete routes from VPNV4 table. + * Here for !IS_BGP_INSTANCE_HIDDEN, + * !(SAFI_MPLS_VPN && AF_IP/AF_IP6), + * we ignore the event for the prefix. + */ + if (BGP_INSTANCE_HIDDEN_DELETE_IN_PROGRESS(bgp, afi, safi)) { if (dest) debug = bgp_debug_bestpath(dest); if (debug) @@ -6444,16 +6453,21 @@ void bgp_cleanup_routes(struct bgp *bgp) if (afi != AFI_L2VPN) { safi_t safi; safi = SAFI_MPLS_VPN; - for (dest = bgp_table_top(bgp->rib[afi][safi]); dest; - dest = bgp_route_next(dest)) { - table = bgp_dest_get_bgp_table_info(dest); - if (table != NULL) { - bgp_cleanup_table(bgp, table, afi, safi); - bgp_table_finish(&table); - bgp_dest_set_bgp_table_info(dest, NULL); - dest = bgp_dest_unlock_node(dest); - - assert(dest); + if (!IS_BGP_INSTANCE_HIDDEN(bgp)) { + for (dest = bgp_table_top(bgp->rib[afi][safi]); + dest; dest = bgp_route_next(dest)) { + table = bgp_dest_get_bgp_table_info( + dest); + if (table != NULL) { + bgp_cleanup_table(bgp, table, + afi, safi); + bgp_table_finish(&table); + bgp_dest_set_bgp_table_info(dest, + NULL); + dest = bgp_dest_unlock_node( + dest); + assert(dest); + } } } safi = SAFI_ENCAP; @@ -12157,7 +12171,7 @@ static int bgp_show(struct vty *vty, struct bgp *bgp, afi_t afi, safi_t safi, bgp = bgp_get_default(); } - if (bgp == NULL) { + if (bgp == NULL || IS_BGP_INSTANCE_HIDDEN(bgp)) { if (!use_json) vty_out(vty, "No BGP process is configured\n"); else @@ -12203,6 +12217,8 @@ static void bgp_show_all_instances_routes_vty(struct vty *vty, afi_t afi, vty_out(vty, "{\n"); for (ALL_LIST_ELEMENTS(bm->bgp, node, nnode, bgp)) { + if (IS_BGP_INSTANCE_HIDDEN(bgp)) + continue; route_output = true; if (use_json) { if (!is_first) @@ -12721,7 +12737,7 @@ static int bgp_show_route(struct vty *vty, struct bgp *bgp, const char *ip_str, { if (!bgp) { bgp = bgp_get_default(); - if (!bgp) { + if (!bgp || IS_BGP_INSTANCE_HIDDEN(bgp)) { if (!use_json) vty_out(vty, "No BGP process is configured\n"); else @@ -14379,7 +14395,7 @@ DEFUN (show_ip_bgp_vpn_all_route_prefix, int idx = 0; char *network = NULL; struct bgp *bgp = bgp_get_default(); - if (!bgp) { + if (!bgp || IS_BGP_INSTANCE_HIDDEN(bgp)) { vty_out(vty, "Can't find default instance\n"); return CMD_WARNING; } @@ -15881,7 +15897,7 @@ static int bgp_clear_damp_route(struct vty *vty, const char *view_name, /* BGP structure lookup. */ if (view_name) { bgp = bgp_lookup_by_name(view_name); - if (bgp == NULL) { + if (bgp == NULL || IS_BGP_INSTANCE_HIDDEN(bgp)) { vty_out(vty, "%% Can't find BGP instance %s\n", view_name); return CMD_WARNING; diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index b6c16a340f..4c2aa8d68a 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -879,6 +879,7 @@ int bgp_vty_return(struct vty *vty, enum bgp_create_error_code ret) switch (ret) { case BGP_SUCCESS: case BGP_CREATED: + case BGP_INSTANCE_EXISTS: case BGP_GR_NO_OPERATION: break; case BGP_ERR_INVALID_VALUE: @@ -1418,7 +1419,7 @@ DEFUN_HIDDEN (bgp_local_mac, seq = strtoul(argv[7]->arg, NULL, 10); bgp = bgp_get_default(); - if (!bgp) { + if (!bgp || IS_BGP_INSTANCE_HIDDEN(bgp)) { vty_out(vty, "Default BGP instance is not there\n"); return CMD_WARNING; } @@ -1458,7 +1459,7 @@ DEFUN_HIDDEN (no_bgp_local_mac, memset(&ip, 0, sizeof(ip)); bgp = bgp_get_default(); - if (!bgp) { + if (!bgp || IS_BGP_INSTANCE_HIDDEN(bgp)) { vty_out(vty, "Default BGP instance is not there\n"); return CMD_WARNING; } @@ -1601,8 +1602,12 @@ DEFUN_NOSH (router_bgp, if (is_new_bgp && inst_type == BGP_INSTANCE_TYPE_DEFAULT) vpn_leak_postchange_all(); - if (inst_type == BGP_INSTANCE_TYPE_VRF) + if (inst_type == BGP_INSTANCE_TYPE_VRF || + IS_BGP_INSTANCE_HIDDEN(bgp)) { bgp_vpn_leak_export(bgp); + UNSET_FLAG(bgp->flags, BGP_FLAG_INSTANCE_HIDDEN); + UNSET_FLAG(bgp->flags, BGP_FLAG_DELETE_IN_PROGRESS); + } /* Pending: handle when user tries to change a view to vrf n vv. */ /* for pre-existing bgp instance, @@ -1674,7 +1679,7 @@ DEFUN (no_router_bgp, argv[idx_asn]->arg); return CMD_WARNING_CONFIG_FAILED; } - if (argc > 4) { + if (argc > 4 && strncmp(argv[4]->arg, "vrf", 3) == 0) { name = argv[idx_vrf]->arg; if (strmatch(argv[idx_vrf - 1]->text, "vrf") && strmatch(name, VRF_DEFAULT_NAME)) @@ -10444,9 +10449,9 @@ DEFPY(af_import_vrf_route_map, af_import_vrf_route_map_cmd, bgp_default = bgp_get_default(); if (!bgp_default) { int32_t ret; - as_t as = bgp->as; + as_t as = AS_UNSPECIFIED; - /* Auto-create assuming the same AS */ + /* Auto-create with AS_UNSPECIFIED, to be filled in later */ ret = bgp_get_vty(&bgp_default, &as, NULL, BGP_INSTANCE_TYPE_DEFAULT, NULL, ASNOTATION_UNDEFINED); @@ -10456,6 +10461,8 @@ DEFPY(af_import_vrf_route_map, af_import_vrf_route_map_cmd, "VRF default is not configured as a bgp instance\n"); return CMD_WARNING; } + + SET_FLAG(bgp_default->flags, BGP_FLAG_INSTANCE_HIDDEN); } vpn_leak_prechange(dir, afi, bgp_get_default(), bgp); @@ -10559,7 +10566,9 @@ DEFPY(bgp_imexport_vrf, bgp_imexport_vrf_cmd, bgp_default = bgp_get_default(); if (!bgp_default) { - /* Auto-create assuming the same AS */ + as = AS_UNSPECIFIED; + + /* Auto-create with AS_UNSPECIFIED, to be filled in later */ ret = bgp_get_vty(&bgp_default, &as, NULL, BGP_INSTANCE_TYPE_DEFAULT, NULL, ASNOTATION_UNDEFINED); @@ -10569,6 +10578,8 @@ DEFPY(bgp_imexport_vrf, bgp_imexport_vrf_cmd, "VRF default is not configured as a bgp instance\n"); return CMD_WARNING; } + + SET_FLAG(bgp_default->flags, BGP_FLAG_INSTANCE_HIDDEN); } vrf_bgp = bgp_lookup_by_name(import_name); @@ -10576,9 +10587,19 @@ DEFPY(bgp_imexport_vrf, bgp_imexport_vrf_cmd, if (strcmp(import_name, VRF_DEFAULT_NAME) == 0) { vrf_bgp = bgp_default; } else { - /* Auto-create assuming the same AS */ + as = AS_UNSPECIFIED; + + /* Auto-create with AS_UNSPECIFIED, fill in later */ ret = bgp_get_vty(&vrf_bgp, &as, import_name, bgp_type, NULL, ASNOTATION_UNDEFINED); + if (ret) { + vty_out(vty, + "VRF %s is not configured as a bgp instance\n", + import_name); + return CMD_WARNING; + } + + SET_FLAG(vrf_bgp->flags, BGP_FLAG_INSTANCE_HIDDEN); /* Auto created VRF instances should be marked * properly, otherwise we have a state after bgpd @@ -11549,7 +11570,7 @@ DEFUN(show_bgp_martian_nexthop_db, show_bgp_martian_nexthop_db_cmd, else bgp = bgp_get_default(); - if (!bgp) { + if (!bgp || IS_BGP_INSTANCE_HIDDEN(bgp)) { vty_out(vty, "%% No BGP process is configured\n"); return CMD_WARNING; } @@ -12781,6 +12802,9 @@ static void bgp_show_all_instances_summary_vty(struct vty *vty, afi_t afi, if (CHECK_FLAG(bgp->vrf_flags, BGP_VRF_AUTO)) continue; + if (IS_BGP_INSTANCE_HIDDEN(bgp)) + continue; + nbr_output = true; if (use_json) { if (!is_first) @@ -16169,6 +16193,9 @@ static void bgp_show_all_instances_neighbors_vty(struct vty *vty, if (CHECK_FLAG(bgp->vrf_flags, BGP_VRF_AUTO)) continue; + if (IS_BGP_INSTANCE_HIDDEN(bgp)) + continue; + nbr_output = true; if (use_json) { if (!(json = json_object_new_object())) { @@ -16824,6 +16851,9 @@ static void bgp_show_all_instances_updgrps_vty(struct vty *vty, afi_t afi, if (CHECK_FLAG(bgp->vrf_flags, BGP_VRF_AUTO)) continue; + if (IS_BGP_INSTANCE_HIDDEN(bgp)) + continue; + if (!uj) vty_out(vty, "\nInstance %s:\n", (bgp->inst_type == BGP_INSTANCE_TYPE_DEFAULT) @@ -16946,7 +16976,7 @@ DEFUN (show_bgp_updgrps_stats, struct bgp *bgp; bgp = bgp_get_default(); - if (bgp) + if (bgp && !IS_BGP_INSTANCE_HIDDEN(bgp)) update_group_show_stats(bgp, vty); return CMD_SUCCESS; @@ -18941,6 +18971,10 @@ static void bgp_config_write_peer_af(struct vty *vty, struct bgp *bgp, char *addr; bool flag_scomm, flag_secomm, flag_slcomm; + /* skip hidden default vrf bgp instance */ + if (IS_BGP_INSTANCE_HIDDEN(bgp)) + return; + /* Skip dynamic neighbors. */ if (peer_dynamic_neighbor(peer)) return; @@ -19246,6 +19280,9 @@ static void bgp_config_write_family(struct vty *vty, struct bgp *bgp, afi_t afi, struct peer_group *group; struct listnode *node, *nnode; + /* skip hidden default vrf bgp instance */ + if (IS_BGP_INSTANCE_HIDDEN(bgp)) + return; vty_frame(vty, " !\n address-family "); if (afi == AFI_IP) { @@ -19428,6 +19465,10 @@ int bgp_config_write(struct vty *vty) if (CHECK_FLAG(bgp->vrf_flags, BGP_VRF_AUTO)) continue; + /* skip hidden default vrf bgp instance */ + if (IS_BGP_INSTANCE_HIDDEN(bgp)) + continue; + /* Router bgp ASN */ vty_out(vty, "router bgp %s", bgp->as_pretty); diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 23ab4d3d91..2b624d1652 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -3421,12 +3421,18 @@ static void bgp_vrf_string_name_delete(void *data) static struct bgp *bgp_create(as_t *as, const char *name, enum bgp_instance_type inst_type, const char *as_pretty, - enum asnotation_mode asnotation) + enum asnotation_mode asnotation, + struct bgp *bgp_old, bool hidden) { struct bgp *bgp; afi_t afi; safi_t safi; + if (hidden) { + bgp = bgp_old; + goto peer_init; + } + bgp = XCALLOC(MTYPE_BGP, sizeof(struct bgp)); bgp->as = *as; if (as_pretty) @@ -3480,18 +3486,24 @@ static struct bgp *bgp_create(as_t *as, const char *name, bgp->peer_self->domainname = XSTRDUP(MTYPE_BGP_PEER_HOST, cmd_domainname_get()); bgp->peer = list_new(); + +peer_init: bgp->peer->cmp = (int (*)(void *, void *))peer_cmp; bgp->peerhash = hash_create(peer_hash_key_make, peer_hash_same, "BGP Peer Hash"); bgp->peerhash->max_size = BGP_PEER_MAX_HASH_SIZE; - bgp->group = list_new(); + if (!hidden) + bgp->group = list_new(); bgp->group->cmp = (int (*)(void *, void *))peer_group_cmp; FOREACH_AFI_SAFI (afi, safi) { - bgp->route[afi][safi] = bgp_table_init(bgp, afi, safi); - bgp->aggregate[afi][safi] = bgp_table_init(bgp, afi, safi); - bgp->rib[afi][safi] = bgp_table_init(bgp, afi, safi); + if (!hidden) { + bgp->route[afi][safi] = bgp_table_init(bgp, afi, safi); + bgp->aggregate[afi][safi] = bgp_table_init(bgp, afi, + safi); + bgp->rib[afi][safi] = bgp_table_init(bgp, afi, safi); + } /* Enable maximum-paths */ bgp_maximum_paths_set(bgp, afi, safi, BGP_PEER_EBGP, @@ -3530,7 +3542,7 @@ static struct bgp *bgp_create(as_t *as, const char *name, bgp->rmap_def_originate_eval_timer = 0; #ifdef ENABLE_BGP_VNC - if (inst_type != BGP_INSTANCE_TYPE_VRF) { + if (inst_type != BGP_INSTANCE_TYPE_VRF && !hidden) { bgp->rfapi = bgp_rfapi_new(bgp); assert(bgp->rfapi); assert(bgp->rfapi_cfg); @@ -3547,9 +3559,11 @@ static struct bgp *bgp_create(as_t *as, const char *name, bgp->vpn_policy[afi].import_vrf = list_new(); bgp->vpn_policy[afi].import_vrf->del = bgp_vrf_string_name_delete; - bgp->vpn_policy[afi].export_vrf = list_new(); - bgp->vpn_policy[afi].export_vrf->del = - bgp_vrf_string_name_delete; + if (!hidden) { + bgp->vpn_policy[afi].export_vrf = list_new(); + bgp->vpn_policy[afi].export_vrf->del = + bgp_vrf_string_name_delete; + } SET_FLAG(bgp->af_flags[afi][SAFI_MPLS_VPN], BGP_VPNVX_RETAIN_ROUTE_TARGET_ALL); } @@ -3567,7 +3581,7 @@ static struct bgp *bgp_create(as_t *as, const char *name, bgp->restart_time, &bgp->t_startup); /* printable name we can use in debug messages */ - if (inst_type == BGP_INSTANCE_TYPE_DEFAULT) { + if (inst_type == BGP_INSTANCE_TYPE_DEFAULT && !hidden) { bgp->name_pretty = XSTRDUP(MTYPE_BGP_NAME, "VRF default"); } else { const char *n; @@ -3595,17 +3609,20 @@ static struct bgp *bgp_create(as_t *as, const char *name, bgp->coalesce_time = BGP_DEFAULT_SUBGROUP_COALESCE_TIME; bgp->default_af[AFI_IP][SAFI_UNICAST] = true; - QOBJ_REG(bgp, bgp); + if (!hidden) + QOBJ_REG(bgp, bgp); update_bgp_group_init(bgp); - /* assign a unique rd id for auto derivation of vrf's RD */ - bf_assign_index(bm->rd_idspace, bgp->vrf_rd_id); + if (!hidden) { + /* assign a unique rd id for auto derivation of vrf's RD */ + bf_assign_index(bm->rd_idspace, bgp->vrf_rd_id); - bgp_evpn_init(bgp); - bgp_evpn_vrf_es_init(bgp); - bgp_pbr_init(bgp); - bgp_srv6_init(bgp); + bgp_evpn_init(bgp); + bgp_evpn_vrf_es_init(bgp); + bgp_pbr_init(bgp); + bgp_srv6_init(bgp); + } /*initilize global GR FSM */ bgp_global_gr_init(bgp); @@ -3743,10 +3760,15 @@ int bgp_handle_socket(struct bgp *bgp, struct vrf *vrf, vrf_id_t old_vrf_id, return bgp_check_main_socket(create, bgp); } -int bgp_lookup_by_as_name_type(struct bgp **bgp_val, as_t *as, const char *name, +int bgp_lookup_by_as_name_type(struct bgp **bgp_val, as_t *as, + const char *as_pretty, + enum asnotation_mode asnotation, const char *name, enum bgp_instance_type inst_type) { struct bgp *bgp; + struct peer *peer = NULL; + struct listnode *node, *nnode; + bool hidden = false; /* Multiple instance check. */ if (name) @@ -3755,14 +3777,28 @@ 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 (IS_BGP_INSTANCE_HIDDEN(bgp) && *as != AS_UNSPECIFIED) + hidden = true; + /* Handle AS number change */ if (bgp->as != *as) { - *as = bgp->as; - return BGP_ERR_AS_MISMATCH; + if (hidden) + bgp_create(as, name, inst_type, as_pretty, + asnotation, bgp, hidden); + + /* Set all peer's local as number with this ASN */ + for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) + peer->local_as = *as; + UNSET_FLAG(bgp->flags, BGP_FLAG_INSTANCE_HIDDEN); + *bgp_val = bgp; + return BGP_INSTANCE_EXISTS; } if (bgp->inst_type != inst_type) return BGP_ERR_INSTANCE_MISMATCH; - return BGP_SUCCESS; + if (hidden) + bgp_create(as, name, inst_type, as_pretty, asnotation, + bgp, hidden); + *bgp_val = bgp; + return BGP_INSTANCE_EXISTS; } *bgp_val = NULL; @@ -3778,11 +3814,13 @@ int bgp_get(struct bgp **bgp_val, as_t *as, const char *name, struct vrf *vrf = NULL; int ret = 0; - ret = bgp_lookup_by_as_name_type(bgp_val, as, name, inst_type); + ret = bgp_lookup_by_as_name_type(bgp_val, as, as_pretty, asnotation, + name, inst_type); if (ret || *bgp_val) return ret; - bgp = bgp_create(as, name, inst_type, as_pretty, asnotation); + bgp = bgp_create(as, name, inst_type, as_pretty, asnotation, NULL, + false); /* * view instances will never work inside of a vrf @@ -4022,6 +4060,15 @@ int bgp_delete(struct bgp *bgp) bgp_damp_disable(bgp, afi, safi); } + if (bgp->inst_type == BGP_INSTANCE_TYPE_DEFAULT && + (bgp_table_top(bgp->rib[AFI_IP][SAFI_MPLS_VPN]) || + bgp_table_top(bgp->rib[AFI_IP6][SAFI_MPLS_VPN]))) { + if (BGP_DEBUG(zebra, ZEBRA)) + zlog_debug( + "Marking the deleting default bgp instance as hidden"); + SET_FLAG(bgp->flags, BGP_FLAG_INSTANCE_HIDDEN); + } + if (BGP_DEBUG(zebra, ZEBRA)) { if (bgp->inst_type == BGP_INSTANCE_TYPE_DEFAULT) zlog_debug("Deleting Default VRF"); @@ -4066,7 +4113,7 @@ int bgp_delete(struct bgp *bgp) peer_delete(peer); } - if (bgp->peer_self) { + if (bgp->peer_self && !IS_BGP_INSTANCE_HIDDEN(bgp)) { peer_delete(bgp->peer_self); bgp->peer_self = NULL; } @@ -4076,7 +4123,8 @@ int bgp_delete(struct bgp *bgp) /* TODO - Other memory may need to be freed - e.g., NHT */ #ifdef ENABLE_BGP_VNC - rfapi_delete(bgp); + if (!IS_BGP_INSTANCE_HIDDEN(bgp)) + rfapi_delete(bgp); #endif /* Free memory allocated with aggregate address configuration. */ @@ -4118,7 +4166,7 @@ int bgp_delete(struct bgp *bgp) } /* Deregister from Zebra, if needed */ - if (IS_BGP_INST_KNOWN_TO_ZEBRA(bgp)) { + if (IS_BGP_INST_KNOWN_TO_ZEBRA(bgp) && !IS_BGP_INSTANCE_HIDDEN(bgp)) { if (BGP_DEBUG(zebra, ZEBRA)) zlog_debug( "%s: deregistering this bgp %s instance from zebra", @@ -4126,17 +4174,19 @@ int bgp_delete(struct bgp *bgp) bgp_zebra_instance_deregister(bgp); } - /* Remove visibility via the master list - there may however still be - * routes to be processed still referencing the struct bgp. - */ - listnode_delete(bm->bgp, bgp); - - /* Free interfaces in this instance. */ - bgp_if_finish(bgp); + if (!IS_BGP_INSTANCE_HIDDEN(bgp)) { + /* Remove visibility via the master list - + * there may however still be routes to be processed + * still referencing the struct bgp. + */ + listnode_delete(bm->bgp, bgp); + /* Free interfaces in this instance. */ + bgp_if_finish(bgp); + } vrf = bgp_vrf_lookup_by_instance_type(bgp); bgp_handle_socket(bgp, vrf, VRF_UNKNOWN, false); - if (vrf) + if (vrf && !IS_BGP_INSTANCE_HIDDEN(bgp)) bgp_vrf_unlink(bgp, vrf); /* Update EVPN VRF pointer */ @@ -4151,7 +4201,22 @@ int bgp_delete(struct bgp *bgp) work_queue_free_and_null(&bgp->process_queue); event_master_free_unused(bm->master); - bgp_unlock(bgp); /* initial reference */ + + if (!IS_BGP_INSTANCE_HIDDEN(bgp)) + bgp_unlock(bgp); /* initial reference */ + else { + for (afi = AFI_IP; afi < AFI_MAX; afi++) { + enum vpn_policy_direction dir; + + if (bgp->vpn_policy[afi].import_vrf) + list_delete(&bgp->vpn_policy[afi].import_vrf); + + dir = BGP_VPN_POLICY_DIR_FROMVPN; + if (bgp->vpn_policy[afi].rtlist[dir]) + ecommunity_free( + &bgp->vpn_policy[afi].rtlist[dir]); + } + } return 0; } diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index def12ee642..3114b141f0 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -550,6 +550,7 @@ struct bgp { #define BGP_FLAG_ENFORCE_FIRST_AS (1ULL << 36) #define BGP_FLAG_DYNAMIC_CAPABILITY (1ULL << 37) #define BGP_FLAG_VNI_DOWN (1ULL << 38) +#define BGP_FLAG_INSTANCE_HIDDEN (1ULL << 39) /* BGP default address-families. * New peers inherit enabled afi/safis from bgp instance. @@ -2152,6 +2153,7 @@ enum bgp_clear_type { enum bgp_create_error_code { BGP_SUCCESS = 0, BGP_CREATED = 1, + BGP_INSTANCE_EXISTS = 2, BGP_ERR_INVALID_VALUE = -1, BGP_ERR_INVALID_FLAG = -2, BGP_ERR_INVALID_AS = -3, @@ -2822,6 +2824,8 @@ extern struct peer *peer_new(struct bgp *bgp); extern struct peer *peer_lookup_in_view(struct vty *vty, struct bgp *bgp, const char *ip_str, bool use_json); extern int bgp_lookup_by_as_name_type(struct bgp **bgp_val, as_t *as, + const char *as_pretty, + enum asnotation_mode asnotation, const char *name, enum bgp_instance_type inst_type); @@ -2863,4 +2867,17 @@ extern void bgp_session_reset_safe(struct peer *peer, struct listnode **nnode); /* clang-format on */ #endif +/* Macro to check if default bgp instance is hidden */ +#define IS_BGP_INSTANCE_HIDDEN(_bgp) \ + (CHECK_FLAG(_bgp->flags, BGP_FLAG_INSTANCE_HIDDEN) && \ + (_bgp->inst_type == BGP_INSTANCE_TYPE_DEFAULT || \ + _bgp->inst_type == BGP_INSTANCE_TYPE_VRF)) + +/* Macro to check if bgp instance delete in-progress and !hidden */ +#define BGP_INSTANCE_HIDDEN_DELETE_IN_PROGRESS(_bgp, _afi, _safi) \ + (CHECK_FLAG(_bgp->flags, BGP_FLAG_DELETE_IN_PROGRESS) && \ + !IS_BGP_INSTANCE_HIDDEN(_bgp) && \ + !(_afi == AFI_IP && _safi == SAFI_MPLS_VPN) && \ + !(_afi == AFI_IP6 && _safi == SAFI_MPLS_VPN)) + #endif /* _QUAGGA_BGPD_H */ From 091abc6b28ddba893d29d90d9e5e816fadb7d188 Mon Sep 17 00:00:00 2001 From: Don Slice Date: Fri, 4 Jun 2021 16:22:24 -0400 Subject: [PATCH 2/6] bgpd: do not allow override ASN unless hidden or auto-created While it's okay to allow overwriting the ASN of a bgp vrf/instance that is either hidden or automatically created, it's dangerous to allow it on explicitly defined instances. If that were allowed, a typo entering the bgp config could take down existing peering, which would be a bad thing. Signed-off-by: Don Slice --- bgpd/bgpd.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 2b624d1652..f139dc3e75 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -3524,7 +3524,8 @@ peer_init: bgp->default_subgroup_pkt_queue_max = BGP_DEFAULT_SUBGROUP_PKT_QUEUE_MAX; bgp_tcp_keepalive_unset(bgp); - bgp_timers_unset(bgp); + if (!hidden) + bgp_timers_unset(bgp); bgp->default_min_holdtime = 0; bgp->restart_time = BGP_DEFAULT_RESTART_TIME; bgp->stalepath_time = BGP_DEFAULT_STALEPATH_TIME; @@ -3781,16 +3782,29 @@ int bgp_lookup_by_as_name_type(struct bgp **bgp_val, as_t *as, hidden = true; /* Handle AS number change */ if (bgp->as != *as) { - if (hidden) - bgp_create(as, name, inst_type, as_pretty, - asnotation, bgp, hidden); + if (hidden || CHECK_FLAG(bgp->vrf_flags, BGP_VRF_AUTO)) { + if (hidden) { + bgp_create(as, name, inst_type, + as_pretty, asnotation, bgp, + hidden); + UNSET_FLAG(bgp->flags, + BGP_FLAG_INSTANCE_HIDDEN); + } else { + bgp->as = *as; + UNSET_FLAG(bgp->vrf_flags, BGP_VRF_AUTO); + } - /* Set all peer's local as number with this ASN */ - for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) - peer->local_as = *as; - UNSET_FLAG(bgp->flags, BGP_FLAG_INSTANCE_HIDDEN); + /* Set all peer's local AS with this ASN */ + for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, + peer)) + peer->local_as = *as; + *bgp_val = bgp; + return BGP_INSTANCE_EXISTS; + } + + *as = bgp->as; *bgp_val = bgp; - return BGP_INSTANCE_EXISTS; + return BGP_ERR_INSTANCE_MISMATCH; } if (bgp->inst_type != inst_type) return BGP_ERR_INSTANCE_MISMATCH; From d4426b62d221f4e15810dbe578de05df8991c991 Mon Sep 17 00:00:00 2001 From: Don Slice Date: Wed, 9 Jun 2021 16:50:20 -0400 Subject: [PATCH 3/6] bgpd: copy source vrf ASN to leaked route and block loops When we leak routes and are using a different ASN in the source vrf from the target vrf, it's possible we could create loops because of an incomplete as-path (missing the source vrf ASN). This fix adds the source vrf ASN and stops the importing of a BGP prefix that has the target ASN in the as-path in the source vrf. Signed-off-by: Don Slice --- bgpd/bgp_mplsvpn.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index 9db73a1bd1..4e5a4150d1 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -34,6 +34,7 @@ #include "bgpd/bgp_nht.h" #include "bgpd/bgp_evpn.h" #include "bgpd/bgp_memory.h" +#include "bgpd/bgp_aspath.h" #ifdef ENABLE_BGP_VNC #include "bgpd/rfapi/rfapi_backend.h" @@ -2156,6 +2157,7 @@ static void vpn_leak_to_vrf_update_onevrf(struct bgp *to_bgp, /* to */ struct bgp *src_vrf; struct interface *ifp = NULL; char rd_buf[RD_ADDRSTRLEN]; + struct aspath *new_aspath; int debug = BGP_DEBUG(vpn, VPN_LEAK_TO_VRF); @@ -2213,6 +2215,32 @@ static void vpn_leak_to_vrf_update_onevrf(struct bgp *to_bgp, /* to */ return; } + bn = bgp_afi_node_get(to_bgp->rib[afi][safi], afi, safi, p, NULL); + + /* Check if leaked route has our asn. If so, don't import it. */ + if (aspath_loop_check(path_vpn->attr->aspath, to_bgp->as)) { + for (bpi = bgp_dest_get_bgp_path_info(bn); bpi; + bpi = bpi->next) { + if (bpi->extra && bpi->extra->vrfleak && + (struct bgp_path_info *)bpi->extra->vrfleak->parent == + path_vpn) { + break; + } + } + + if (bpi) { + if (debug) + zlog_debug("%s: blocking import of %p, as-path match", + __func__, bpi); + bgp_aggregate_decrement(to_bgp, p, bpi, afi, safi); + bgp_path_info_delete(bn, bpi); + bgp_process(to_bgp, bn, bpi, afi, safi); + } + bgp_dest_unlock_node(bn); + + return; + } + if (debug) zlog_debug("%s: updating RD %s, %pFX to %s", __func__, rd_buf, p, to_bgp->name_pretty); @@ -2365,6 +2393,20 @@ static void vpn_leak_to_vrf_update_onevrf(struct bgp *to_bgp, /* to */ nexthop_self_flag = 0; } + /* + * if the asn values are different, copy the asn of the source vrf + * into the entry before importing. This helps with as-path loop + * detection + */ + if (path_vpn->extra && path_vpn->extra->vrfleak && + (to_bgp->as != path_vpn->extra->vrfleak->bgp_orig->as)) { + new_aspath = aspath_dup(static_attr.aspath); + new_aspath = + aspath_add_seq(new_aspath, + path_vpn->extra->vrfleak->bgp_orig->as); + static_attr.aspath = new_aspath; + } + new_attr = bgp_attr_intern(&static_attr); bgp_attr_flush(&static_attr); From dc4440bdb2455c5350d4db992d196843450fbf7e Mon Sep 17 00:00:00 2001 From: Don Slice Date: Mon, 7 Jun 2021 14:14:45 -0400 Subject: [PATCH 4/6] bgpd: copy asn for prefixes imported as type5 from a vrf When a prefix in a vrf is imported into evpn as a type5, copy the asn of the source to make sure it is reflected in the target vrf. Ticket: cumuluslinux-2554562 Signed-off-by: Don Slice --- bgpd/bgp_evpn.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index db93640635..986158ce33 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -1611,12 +1611,16 @@ static int update_evpn_type5_route_entry(struct bgp *bgp_evpn, struct bgp_labels bgp_labels = {}; struct bgp_path_info *local_pi = NULL; struct bgp_path_info *tmp_pi = NULL; + struct aspath *new_aspath; + struct attr static_attr = { 0 }; *route_changed = 0; /* See if this is an update of an existing route, or a new add. */ local_pi = bgp_evpn_route_get_local_path(bgp_evpn, dest); + static_attr = *attr; + /* * create a new route entry if one doesn't exist. * Otherwise see if route attr has changed @@ -1626,8 +1630,19 @@ static int update_evpn_type5_route_entry(struct bgp *bgp_evpn, /* route has changed as this is the first entry */ *route_changed = 1; + /* + * if the asn values are different, copy the as of + * source vrf to the target entry + */ + if (bgp_vrf->as != bgp_evpn->as) { + new_aspath = aspath_dup(static_attr.aspath); + new_aspath = aspath_add_seq(new_aspath, bgp_vrf->as); + static_attr.aspath = new_aspath; + } + /* Add (or update) attribute to hash. */ - attr_new = bgp_attr_intern(attr); + attr_new = bgp_attr_intern(&static_attr); + bgp_attr_flush(&static_attr); /* create the route info from attribute */ pi = info_make(ZEBRA_ROUTE_BGP, BGP_ROUTE_STATIC, 0, From ffa0fd5c43a476010226e47f6aea357e81b43a9f Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 19 Jul 2021 10:29:42 -0400 Subject: [PATCH 5/6] bgpd: Fixup crash when leaking from default vrf for mpls vpn's When we get a update on a route that we already have information on from another router and that route has been leaked ensure that we do not crash when trying to releak the code when we may want to modify the as path. Signed-off-by: Donald Sharp --- bgpd/bgp_mplsvpn.c | 1 + 1 file changed, 1 insertion(+) diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index 4e5a4150d1..13da55ffb7 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -2399,6 +2399,7 @@ static void vpn_leak_to_vrf_update_onevrf(struct bgp *to_bgp, /* to */ * detection */ if (path_vpn->extra && path_vpn->extra->vrfleak && + path_vpn->extra->vrfleak->bgp_orig && (to_bgp->as != path_vpn->extra->vrfleak->bgp_orig->as)) { new_aspath = aspath_dup(static_attr.aspath); new_aspath = From bb977d13b865243c20e1792f43e6653562bcbce2 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Mon, 23 Sep 2024 16:55:31 +0300 Subject: [PATCH 6/6] bgpd: Delete EVPN VRF instace if it's not a hidden instance only ``` ==5445==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x7ff4c6bedb19 bp 0x7ffc95f2e400 sp 0x7ffc95f2e3c0 T0) ==5445==The signal is caused by a READ memory access. ==5445==Hint: address points to the zero page. #0 0x7ff4c6bedb19 in hash_iterate lib/hash.c:246 #1 0x5618f41f5f59 in bgp_evpn_nh_finish bgpd/bgp_evpn_mh.c:4663 #2 0x5618f41dcbe8 in bgp_evpn_vrf_delete bgpd/bgp_evpn.c:7336 #3 0x5618f43bdd35 in bgp_delete bgpd/bgpd.c:4098 #4 0x5618f417ef6e in bgp_exit bgpd/bgp_main.c:206 #5 0x5618f417ef6e in sigint bgpd/bgp_main.c:164 #6 0x7ff4c6cac6c4 in frr_sigevent_process lib/sigevent.c:117 #7 0x7ff4c6cd8258 in event_fetch lib/event.c:1767 #8 0x7ff4c6c0dcbc in frr_run lib/libfrr.c:1230 #9 0x5618f418080d in main bgpd/bgp_main.c:555 #10 0x7ff4c670c249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 #11 0x7ff4c670c304 in __libc_start_main_impl ../csu/libc-start.c:360 #12 0x5618f417ea20 in _start (/usr/lib/frr/bgpd+0x2e4a20) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV lib/hash.c:246 in hash_iterate ``` Signed-off-by: Donatas Abraitis --- bgpd/bgpd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index f139dc3e75..43aa6344be 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -4095,7 +4095,8 @@ int bgp_delete(struct bgp *bgp) } /* unmap from RT list */ - bgp_evpn_vrf_delete(bgp); + if (!IS_BGP_INSTANCE_HIDDEN(bgp)) + bgp_evpn_vrf_delete(bgp); /* unmap bgp vrf label */ vpn_leak_zebra_vrf_label_withdraw(bgp, AFI_IP);