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: b98caac ("sharpd, zebra, lib, doc: add recursive support for protocol nhid")
Signed-off-by: Philippe Guibert <[email protected]>

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;

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);

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;

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++)