From f03251e04722d149eae36b9932236a12733cbd63 Mon Sep 17 00:00:00 2001 From: Philippe Guibert Date: Thu, 14 Mar 2024 15:06:29 +0100 Subject: [PATCH] bgpd: fix nhg loop in bgp_default_originate() test. The bgp_default_originate test fails. The ZEBRA daemon goes in OOM, since he keeps looping over the resolution of a NHG which is updated. When sending a route with recursive resolution to ZEBRA, ZEBRA performs some checks with the prefix that are not available when pushing only the NHG: to avoid resolving over a same prefix, the nexthop_active() function compares the prefix to install with the current path used to validate the prefix, and invalidates the route when this occurs. Similarly, if the path to resolve is a default route, the control is left to ZEBRA, and this detection must be done recursively, when parsing the paths of the prefix to install. To avoid all those corner cases, BGP will detect when a path is resolved over a default route, or when a path resolves over its own prefix, and will use the regular ROUTE_ADD/ROUTE_DELETE instead of using the NHG approach. Fixes: b98caaca6162 ("sharpd, zebra, lib, doc: add recursive support for protocol nhid") Signed-off-by: Philippe Guibert --- bgpd/bgp_nexthop.h | 1 + bgpd/bgp_nhg.c | 115 +++++++++++++++++++++++++++++++++++++++++++++ bgpd/bgp_nht.c | 3 ++ bgpd/bgp_zebra.c | 23 +++++++++ 4 files changed, 142 insertions(+) diff --git a/bgpd/bgp_nexthop.h b/bgpd/bgp_nexthop.h index 830883872e77..430c8f17e890 100644 --- a/bgpd/bgp_nexthop.h +++ b/bgpd/bgp_nexthop.h @@ -90,6 +90,7 @@ struct bgp_nexthop_cache { struct bgp_nexthop_cache_head *tree; struct prefix prefix; + struct prefix resolved_prefix; void *nht_info; /* In BGP, peer session */ LIST_HEAD(path_list, bgp_path_info) paths; unsigned int path_count; diff --git a/bgpd/bgp_nhg.c b/bgpd/bgp_nhg.c index 64b0edbcdad1..134f2b2bd518 100644 --- a/bgpd/bgp_nhg.c +++ b/bgpd/bgp_nhg.c @@ -749,6 +749,112 @@ static void bgp_nhg_detach_nexthop(struct bgp_nhg_cache *nhg) bgp_nhg_del_nhg(nhg); } +/* called whenever a nhg is resolved over ourselves + * nexthop_active() detects compares prefix to install with the resolved route + * but with nhg, the prefix is not available, and we lose the loop detection + * this code replaces nhgs with regular route_add/delete to avoid this corner case + * for each path to install, use route_add/delete when route resolved matches the prefix + * for each path, we assume dependent paths are active, and are pushed again to zebra + */ +static bool bgp_nhg_try_detach_if_same_prefix(struct bgp_nhg_cache *nhg, + struct prefix *resolved_prefix) +{ + struct bgp_nhg_connected *rb_node_dep = NULL; + struct bgp_nhg_cache *dep_nhg; + struct bgp_path_info *path, *safe; + const struct prefix *p; + + frr_each_safe (bgp_nhg_connected_tree, &nhg->nhg_dependents, + rb_node_dep) { + dep_nhg = rb_node_dep->nhg; + LIST_FOREACH_SAFE (path, &(dep_nhg->paths), nhg_cache_thread, + safe) { + p = bgp_dest_get_prefix(path->net); + if (path->bgp_nhg_nexthop == nhg && + prefix_same(resolved_prefix, p) && + ((p->family == AF_INET && + (p->prefixlen != IPV4_MAX_BITLEN)) || + (p->family == AF_INET6 && + (p->prefixlen != IPV6_MAX_BITLEN)))) { + if (BGP_DEBUG(nexthop_group, NEXTHOP_GROUP)) + zlog_debug(" %s: %pFX, matched against ourself and prefix length is not max bit length", + __func__, p); + /* nhg = pi->nhg_nexthop is detached, + * nhg will not be suppressed when bgp_nhg_path_unlink() is called + */ + bgp_nhg_path_nexthop_unlink(path, false); + bgp_nhg_path_unlink(path); + /* path should still be active */ + if (bgp_dest_get_bgp_table_info(path->net)->bgp) + bgp_zebra_announce( + path->net, p, path, + bgp_dest_get_bgp_table_info( + path->net) + ->bgp, + bgp_dest_get_bgp_table_info( + path->net) + ->afi, + bgp_dest_get_bgp_table_info( + path->net) + ->safi); + } + } + } + if (LIST_EMPTY(&(nhg->paths))) { + bgp_nhg_del_nhg(nhg); + return true; + } + return false; +} + +/* called whenever a nhg is resolved over default route: + * nexthop_active() detects resolution over default route by comparing to the matching routes + * but with nhg, we may lose the ability to detect a looped resolution + * this code replaced nhgs with regular route_add/delete to avoid this corner case + * for each path, we assume dependent paths are active, and are pushed again to zebra + */ +static bool bgp_nhg_detach_paths(struct bgp_nhg_cache *nhg) +{ + struct bgp_nhg_connected *rb_node_dep = NULL; + struct bgp_nhg_cache *dep_nhg; + struct bgp_path_info *path, *safe; + const struct prefix *p; + + frr_each_safe (bgp_nhg_connected_tree, &nhg->nhg_dependents, + rb_node_dep) { + dep_nhg = rb_node_dep->nhg; + LIST_FOREACH_SAFE (path, &(dep_nhg->paths), nhg_cache_thread, + safe) { + p = bgp_dest_get_prefix(path->net); + if (BGP_DEBUG(nexthop_group, NEXTHOP_GROUP)) + zlog_debug(" :%s: %pFX Resolved against default route", + __func__, p); + /* nhg = pi->nhg_nexthop is detached, + * nhg will not be suppressed when bgp_nhg_path_unlink() is called + */ + bgp_nhg_path_nexthop_unlink(path, false); + bgp_nhg_path_unlink(path); + /* path should still be active */ + if (bgp_dest_get_bgp_table_info(path->net)->bgp) + bgp_zebra_announce(path->net, p, path, + bgp_dest_get_bgp_table_info( + path->net) + ->bgp, + bgp_dest_get_bgp_table_info( + path->net) + ->afi, + bgp_dest_get_bgp_table_info( + path->net) + ->safi); + } + } + if (LIST_EMPTY(&(nhg->paths))) { + bgp_nhg_del_nhg(nhg); + return true; + } + return false; +} + void bgp_nhg_refresh_by_nexthop(struct bgp_nexthop_cache *bnc) { struct bgp_nhg_cache *nhg; @@ -803,6 +909,15 @@ void bgp_nhg_refresh_by_nexthop(struct bgp_nexthop_cache *bnc) bgp_nhg_detach_nexthop(nhg); continue; } + + if (is_default_prefix(&bnc->resolved_prefix) && + bgp_nhg_detach_paths(nhg)) + continue; + + if (bgp_nhg_try_detach_if_same_prefix(nhg, + &bnc->resolved_prefix)) + continue; + if (BGP_DEBUG(nexthop_group, NEXTHOP_GROUP)) zlog_debug("NHG %u, VRF %u : IGP change detected with NH %pFX SRTE %u", nhg->id, vrf_id, p, srte_color); diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c index 732458255a75..daa5fc62a18c 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -650,6 +650,8 @@ static void bgp_process_nexthop_update(struct bgp_nexthop_cache *bnc, } else if (nhr->nexthop_num) { struct peer *peer = bnc->nht_info; + prefix_copy(&bnc->resolved_prefix, &nhr->prefix); + /* notify bgp fsm if nbr ip goes from invalid->valid */ if (!bnc->nexthop_num) UNSET_FLAG(bnc->flags, BGP_NEXTHOP_PEER_NOTIFIED); @@ -755,6 +757,7 @@ static void bgp_process_nexthop_update(struct bgp_nexthop_cache *bnc, } } } else { + memset(&bnc->resolved_prefix, 0, sizeof(bnc->resolved_prefix)); bnc->flags &= ~BGP_NEXTHOP_EVPN_INCOMPLETE; bnc->flags &= ~BGP_NEXTHOP_VALID; bnc->flags &= ~BGP_NEXTHOP_LABELED_VALID; diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c index 6bee766d0243..53e62031e319 100644 --- a/bgpd/bgp_zebra.c +++ b/bgpd/bgp_zebra.c @@ -1508,6 +1508,29 @@ void bgp_zebra_announce_parse_nexthop(struct bgp_path_info *info, * or blackhole routes */ for (i = 0; i < *valid_nh_count; i++) { + if (p_mpinfo[i] && p_mpinfo[i]->nexthop && + is_default_prefix(&p_mpinfo[i]->nexthop->resolved_prefix)) { + if (BGP_DEBUG(nexthop_group, NEXTHOP_GROUP)) + zlog_debug(" :%s: %pFX Resolved against default route", + __func__, p); + for (i = 0; i < *valid_nh_count; i++) + bgp_nhg_path_unlink(p_mpinfo[i]); + return; + } + if (p_mpinfo[i] && p_mpinfo[i]->nexthop && + prefix_same(p, &p_mpinfo[i]->nexthop->resolved_prefix)) { + if ((p->family == AF_INET && + (p->prefixlen != IPV4_MAX_BITLEN)) || + (p->family == AF_INET6 && + (p->prefixlen != IPV6_MAX_BITLEN))) { + if (BGP_DEBUG(nexthop_group, NEXTHOP_GROUP)) + zlog_debug(" %s: %pFX, Matched against ourself and prefix length is not max bit length", + __func__, p); + for (i = 0; i < *valid_nh_count; i++) + bgp_nhg_path_unlink(p_mpinfo[i]); + } + return; + } if (api->nexthops[i].type == NEXTHOP_TYPE_IFINDEX || api->nexthops[i].type == NEXTHOP_TYPE_BLACKHOLE) { for (i = 0; i < *valid_nh_count; i++)