From 2143f9c12c16d3a6ba06f4e2b8679a4d4186b951 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 21 Jul 2022 15:42:51 -0400 Subject: [PATCH] bgpd: LL peers need bnc's per peer FRR should create a bnc per peer. Not have one's that write over others. Currently when FRR has multiple Interface based peering, BGP wa creating a single BNC. This is insufficient in that we were accidently overwriting the one LL with other data. This causes issues when there are multiple and there is weird starting issues with those interfaces that you are peering over. Signed-off-by: Donald Sharp (cherry picked from commit 35aae5c9bcf91703b74464d2b0dca6504bd37e39) --- bgpd/bgp_evpn.c | 2 +- bgpd/bgp_nexthop.c | 15 ++++-- bgpd/bgp_nexthop.h | 6 ++- bgpd/bgp_nht.c | 114 ++++++++++++++++++++++++++++++++++----------- 4 files changed, 103 insertions(+), 34 deletions(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index bbbe538acc..395111e1d2 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -6428,7 +6428,7 @@ static void bgp_evpn_remote_ip_process_nexthops(struct bgpevpn *vpn, return; tree = &vpn->bgp_vrf->nexthop_cache_table[afi]; - bnc = bnc_find(tree, &p, 0); + bnc = bnc_find(tree, &p, 0, 0); if (!bnc || !bnc->is_evpn_gwip_nexthop) return; diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c index c6043807dd..e1fcc743ec 100644 --- a/bgpd/bgp_nexthop.c +++ b/bgpd/bgp_nexthop.c @@ -56,6 +56,11 @@ int bgp_nexthop_cache_compare(const struct bgp_nexthop_cache *a, if (a->srte_color > b->srte_color) return 1; + if (a->ifindex < b->ifindex) + return -1; + if (a->ifindex > b->ifindex) + return 1; + return prefix_cmp(&a->prefix, &b->prefix); } @@ -70,13 +75,15 @@ void bnc_nexthop_free(struct bgp_nexthop_cache *bnc) } struct bgp_nexthop_cache *bnc_new(struct bgp_nexthop_cache_head *tree, - struct prefix *prefix, uint32_t srte_color) + struct prefix *prefix, uint32_t srte_color, + ifindex_t ifindex) { struct bgp_nexthop_cache *bnc; bnc = XCALLOC(MTYPE_BGP_NEXTHOP_CACHE, sizeof(struct bgp_nexthop_cache)); bnc->prefix = *prefix; + bnc->ifindex = ifindex; bnc->srte_color = srte_color; bnc->tree = tree; LIST_INIT(&(bnc->paths)); @@ -106,7 +113,8 @@ void bnc_free(struct bgp_nexthop_cache *bnc) } struct bgp_nexthop_cache *bnc_find(struct bgp_nexthop_cache_head *tree, - struct prefix *prefix, uint32_t srte_color) + struct prefix *prefix, uint32_t srte_color, + ifindex_t ifindex) { struct bgp_nexthop_cache bnc = {}; @@ -115,6 +123,7 @@ struct bgp_nexthop_cache *bnc_find(struct bgp_nexthop_cache_head *tree, bnc.prefix = *prefix; bnc.srte_color = srte_color; + bnc.ifindex = ifindex; return bgp_nexthop_cache_find(tree, &bnc); } @@ -915,7 +924,7 @@ static int show_ip_bgp_nexthop_table(struct vty *vty, const char *name, } tree = import_table ? &bgp->import_check_table : &bgp->nexthop_cache_table; - bnc = bnc_find(tree[family2afi(nhop.family)], &nhop, 0); + bnc = bnc_find(tree[family2afi(nhop.family)], &nhop, 0, 0); if (!bnc) { vty_out(vty, "specified nexthop does not have entry\n"); return CMD_SUCCESS; diff --git a/bgpd/bgp_nexthop.h b/bgpd/bgp_nexthop.h index 16c2b6c65a..9d653ef4dc 100644 --- a/bgpd/bgp_nexthop.h +++ b/bgpd/bgp_nexthop.h @@ -152,12 +152,14 @@ extern bool bgp_nexthop_self(struct bgp *bgp, afi_t afi, uint8_t type, struct bgp_dest *dest); extern struct bgp_nexthop_cache *bnc_new(struct bgp_nexthop_cache_head *tree, struct prefix *prefix, - uint32_t srte_color); + uint32_t srte_color, + ifindex_t ifindex); extern bool bnc_existing_for_prefix(struct bgp_nexthop_cache *bnc); extern void bnc_free(struct bgp_nexthop_cache *bnc); extern struct bgp_nexthop_cache *bnc_find(struct bgp_nexthop_cache_head *tree, struct prefix *prefix, - uint32_t srte_color); + uint32_t srte_color, + ifindex_t ifindex); extern void bnc_nexthop_free(struct bgp_nexthop_cache *bnc); extern const char *bnc_str(struct bgp_nexthop_cache *bnc, char *buf, int size); extern void bgp_scan_init(struct bgp *bgp); diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c index 3ab67a6181..e03e83db2c 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -84,9 +84,10 @@ static void bgp_unlink_nexthop_check(struct bgp_nexthop_cache *bnc) if (LIST_EMPTY(&(bnc->paths)) && !bnc->nht_info) { if (BGP_DEBUG(nht, NHT)) { char buf[PREFIX2STR_BUFFER]; - zlog_debug("%s: freeing bnc %s(%u)(%s)", __func__, + zlog_debug("%s: freeing bnc %s(%d)(%u)(%s)", __func__, bnc_str(bnc, buf, PREFIX2STR_BUFFER), - bnc->srte_color, bnc->bgp->name_pretty); + bnc->ifindex, bnc->srte_color, + bnc->bgp->name_pretty); } /* only unregister if this is the last nh for this prefix*/ if (!bnc_existing_for_prefix(bnc)) @@ -113,17 +114,32 @@ void bgp_replace_nexthop_by_peer(struct peer *from, struct peer *to) struct prefix pt; struct bgp_nexthop_cache *bncp, *bnct; afi_t afi; + ifindex_t ifindex = 0; if (!sockunion2hostprefix(&from->su, &pp)) return; + /* + * Gather the ifindex for if up/down events to be + * tagged into this fun + */ + if (from->conf_if && IN6_IS_ADDR_LINKLOCAL(&from->su.sin6.sin6_addr)) + ifindex = from->su.sin6.sin6_scope_id; + afi = family2afi(pp.family); - bncp = bnc_find(&from->bgp->nexthop_cache_table[afi], &pp, 0); + bncp = bnc_find(&from->bgp->nexthop_cache_table[afi], &pp, 0, ifindex); if (!sockunion2hostprefix(&to->su, &pt)) return; - bnct = bnc_find(&to->bgp->nexthop_cache_table[afi], &pt, 0); + /* + * Gather the ifindex for if up/down events to be + * tagged into this fun + */ + ifindex = 0; + if (to->conf_if && IN6_IS_ADDR_LINKLOCAL(&to->su.sin6.sin6_addr)) + ifindex = to->su.sin6.sin6_scope_id; + bnct = bnc_find(&to->bgp->nexthop_cache_table[afi], &pt, 0, ifindex); if (bnct != bncp) return; @@ -137,11 +153,17 @@ void bgp_unlink_nexthop_by_peer(struct peer *peer) struct prefix p; struct bgp_nexthop_cache *bnc; afi_t afi = family2afi(peer->su.sa.sa_family); + ifindex_t ifindex = 0; if (!sockunion2hostprefix(&peer->su, &p)) return; - - bnc = bnc_find(&peer->bgp->nexthop_cache_table[afi], &p, 0); + /* + * Gather the ifindex for if up/down events to be + * tagged into this fun + */ + if (afi == AFI_IP6 && IN6_IS_ADDR_LINKLOCAL(&peer->su.sin6.sin6_addr)) + ifindex = peer->su.sin6.sin6_scope_id; + bnc = bnc_find(&peer->bgp->nexthop_cache_table[afi], &p, 0, ifindex); if (!bnc) return; @@ -206,9 +228,18 @@ int bgp_find_or_add_nexthop(struct bgp *bgp_route, struct bgp *bgp_nexthop, * Gather the ifindex for if up/down events to be * tagged into this fun */ - if (afi == AFI_IP6 - && IN6_IS_ADDR_LINKLOCAL(&peer->su.sin6.sin6_addr)) + if (afi == AFI_IP6 && + IN6_IS_ADDR_LINKLOCAL(&peer->su.sin6.sin6_addr)) { ifindex = peer->su.sin6.sin6_scope_id; + if (ifindex == 0) { + if (BGP_DEBUG(nht, NHT)) { + zlog_debug( + "%s: Unable to locate ifindex, waiting till we have one", + peer->conf_if); + } + return 0; + } + } if (!sockunion2hostprefix(&peer->su, &p)) { if (BGP_DEBUG(nht, NHT)) { @@ -226,28 +257,27 @@ int bgp_find_or_add_nexthop(struct bgp *bgp_route, struct bgp *bgp_nexthop, else tree = &bgp_nexthop->nexthop_cache_table[afi]; - bnc = bnc_find(tree, &p, srte_color); + bnc = bnc_find(tree, &p, srte_color, ifindex); if (!bnc) { - bnc = bnc_new(tree, &p, srte_color); + bnc = bnc_new(tree, &p, srte_color, ifindex); bnc->bgp = bgp_nexthop; - bnc->ifindex = ifindex; if (BGP_DEBUG(nht, NHT)) { char buf[PREFIX2STR_BUFFER]; - zlog_debug("Allocated bnc %s(%u)(%s) peer %p", + zlog_debug("Allocated bnc %s(%d)(%u)(%s) peer %p", bnc_str(bnc, buf, PREFIX2STR_BUFFER), - bnc->srte_color, bnc->bgp->name_pretty, - peer); + bnc->ifindex, bnc->srte_color, + bnc->bgp->name_pretty, peer); } } else { if (BGP_DEBUG(nht, NHT)) { char buf[PREFIX2STR_BUFFER]; zlog_debug( - "Found existing bnc %s(%s) flags 0x%x ifindex %d #paths %d peer %p", + "Found existing bnc %s(%d)(%s) flags 0x%x ifindex %d #paths %d peer %p", bnc_str(bnc, buf, PREFIX2STR_BUFFER), - bnc->bgp->name_pretty, bnc->flags, bnc->ifindex, - bnc->path_count, bnc->nht_info); + bnc->ifindex, bnc->bgp->name_pretty, bnc->flags, + bnc->ifindex, bnc->path_count, bnc->nht_info); } } @@ -351,15 +381,21 @@ void bgp_delete_connected_nexthop(afi_t afi, struct peer *peer) { struct bgp_nexthop_cache *bnc; struct prefix p; + ifindex_t ifindex = 0; if (!peer) return; if (!sockunion2hostprefix(&peer->su, &p)) return; - + /* + * Gather the ifindex for if up/down events to be + * tagged into this fun + */ + if (afi == AFI_IP6 && IN6_IS_ADDR_LINKLOCAL(&peer->su.sin6.sin6_addr)) + ifindex = peer->su.sin6.sin6_scope_id; bnc = bnc_find(&peer->bgp->nexthop_cache_table[family2afi(p.family)], - &p, 0); + &p, 0, ifindex); if (!bnc) { if (BGP_DEBUG(nht, NHT)) zlog_debug( @@ -408,9 +444,9 @@ static void bgp_process_nexthop_update(struct bgp_nexthop_cache *bnc, char bnc_buf[BNC_FLAG_DUMP_SIZE]; zlog_debug( - "%s(%u): Rcvd NH update %pFX(%u) - metric %d/%d #nhops %d/%d flags %s", + "%s(%u): Rcvd NH update %pFX(%u)%u) - metric %d/%d #nhops %d/%d flags %s", bnc->bgp->name_pretty, bnc->bgp->vrf_id, &nhr->prefix, - bnc->srte_color, nhr->metric, bnc->metric, + bnc->ifindex, bnc->srte_color, nhr->metric, bnc->metric, nhr->nexthop_num, bnc->nexthop_num, bgp_nexthop_dump_bnc_flags(bnc, bnc_buf, sizeof(bnc_buf))); @@ -659,15 +695,22 @@ void bgp_nht_interface_events(struct peer *peer) struct bgp_nexthop_cache_head *table; struct bgp_nexthop_cache *bnc; struct prefix p; + ifindex_t ifindex = 0; if (!IN6_IS_ADDR_LINKLOCAL(&peer->su.sin6.sin6_addr)) return; if (!sockunion2hostprefix(&peer->su, &p)) return; + /* + * Gather the ifindex for if up/down events to be + * tagged into this fun + */ + if (peer->conf_if && IN6_IS_ADDR_LINKLOCAL(&peer->su.sin6.sin6_addr)) + ifindex = peer->su.sin6.sin6_scope_id; table = &bgp->nexthop_cache_table[AFI_IP6]; - bnc = bnc_find(table, &p, 0); + bnc = bnc_find(table, &p, 0, ifindex); if (!bnc) return; @@ -703,7 +746,7 @@ void bgp_parse_nexthop_update(int command, vrf_id_t vrf_id) afi = family2afi(match.family); tree = &bgp->nexthop_cache_table[afi]; - bnc_nhc = bnc_find(tree, &match, nhr.srte_color); + bnc_nhc = bnc_find(tree, &match, nhr.srte_color, 0); if (!bnc_nhc) { if (BGP_DEBUG(nht, NHT)) zlog_debug( @@ -714,7 +757,7 @@ void bgp_parse_nexthop_update(int command, vrf_id_t vrf_id) tree = &bgp->import_check_table[afi]; - bnc_import = bnc_find(tree, &match, nhr.srte_color); + bnc_import = bnc_find(tree, &match, nhr.srte_color, 0); if (!bnc_import) { if (BGP_DEBUG(nht, NHT)) zlog_debug( @@ -982,8 +1025,9 @@ void evaluate_paths(struct bgp_nexthop_cache *bnc) bnc_str(bnc, buf, PREFIX2STR_BUFFER); zlog_debug( - "NH update for %s(%u)(%s) - flags %s chgflags %s- evaluate paths", - buf, bnc->srte_color, bnc->bgp->name_pretty, + "NH update for %s(%d)(%u)(%s) - flags %s chgflags %s- evaluate paths", + buf, bnc->ifindex, bnc->srte_color, + bnc->bgp->name_pretty, bgp_nexthop_dump_bnc_flags(bnc, bnc_buf, sizeof(bnc_buf)), bgp_nexthop_dump_bnc_change_flags(bnc, chg_buf, @@ -1190,6 +1234,7 @@ void bgp_nht_reg_enhe_cap_intfs(struct peer *peer) struct nexthop *nhop; struct interface *ifp; struct prefix p; + ifindex_t ifindex = 0; if (peer->ifp) return; @@ -1203,8 +1248,14 @@ void bgp_nht_reg_enhe_cap_intfs(struct peer *peer) if (p.family != AF_INET6) return; + /* + * Gather the ifindex for if up/down events to be + * tagged into this fun + */ + if (peer->conf_if && IN6_IS_ADDR_LINKLOCAL(&peer->su.sin6.sin6_addr)) + ifindex = peer->su.sin6.sin6_scope_id; - bnc = bnc_find(&bgp->nexthop_cache_table[AFI_IP6], &p, 0); + bnc = bnc_find(&bgp->nexthop_cache_table[AFI_IP6], &p, 0, ifindex); if (!bnc) return; @@ -1231,6 +1282,7 @@ void bgp_nht_dereg_enhe_cap_intfs(struct peer *peer) struct nexthop *nhop; struct interface *ifp; struct prefix p; + ifindex_t ifindex = 0; if (peer->ifp) return; @@ -1245,8 +1297,14 @@ void bgp_nht_dereg_enhe_cap_intfs(struct peer *peer) if (p.family != AF_INET6) return; + /* + * Gather the ifindex for if up/down events to be + * tagged into this fun + */ + if (peer->conf_if && IN6_IS_ADDR_LINKLOCAL(&peer->su.sin6.sin6_addr)) + ifindex = peer->su.sin6.sin6_scope_id; - bnc = bnc_find(&bgp->nexthop_cache_table[AFI_IP6], &p, 0); + bnc = bnc_find(&bgp->nexthop_cache_table[AFI_IP6], &p, 0, ifindex); if (!bnc) return; -- 2.39.5