From e1db302d50aa5495ac81aa8fec9d608a492b5fad 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 | 104 +++++++++++++++++++++++++++++++++++++++++++++ bgpd/bgp_nht.c | 3 ++ bgpd/bgp_zebra.c | 23 ++++++++++ 4 files changed, 131 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 f86e91d2f4e6..903fa9023b9a 100644 --- a/bgpd/bgp_nhg.c +++ b/bgpd/bgp_nhg.c @@ -750,6 +750,101 @@ 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_groups, + 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_route_install(path->net, path, + bgp_dest_get_bgp_table_info( + path->net) + ->bgp, + true); + } + } + } + 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_groups, + 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_route_install(path->net, path, + bgp_dest_get_bgp_table_info( + path->net) + ->bgp, + true); + } + } + 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; @@ -804,6 +899,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 1f64136aa8df..14d6c350ab9d 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++)