bgpd: combine import_check_table and nexthop_check_table

In zebra, the import check table and the nexthop check tables were combined. This leaves an issue where when bgp happens to have a tracked address in both the import check table and the nexthop track table that are the same address. When the the item is removed from one table the call to remove it from zebra removes tracking for the other table. Combine the two tables together and keep track where they came from for processing in bgpd. Signed-off-by: Donald Sharp <[email protected]>
donaldsharp · Oct 25, 2023 · 4a43b81 · 4a43b81
1 parent f239b0f
commit 4a43b81
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 41 deletions.
diff --git a/bgpd/bgp_nexthop.c b/bgpd/bgp_nexthop.c
@@ -58,7 +58,8 @@ 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,
-				  ifindex_t ifindex)
+				  ifindex_t ifindex, bool import_check_table,
+				  bool nexthop_check_table)
 {
 	struct bgp_nexthop_cache *bnc;
 
@@ -68,6 +69,9 @@ struct bgp_nexthop_cache *bnc_new(struct bgp_nexthop_cache_head *tree,
 	bnc->ifindex_ipv6_ll = ifindex;
 	bnc->srte_color = srte_color;
 	bnc->tree = tree;
+	bnc->import_check_table = import_check_table;
+	bnc->nexthop_check_table = nexthop_check_table;
+
 	LIST_INIT(&(bnc->paths));
 	bgp_nexthop_cache_add(tree, bnc);
 
@@ -968,7 +972,7 @@ static void bgp_show_nexthops_detail(struct vty *vty, struct bgp *bgp,
 
 static void bgp_show_nexthop(struct vty *vty, struct bgp *bgp,
 			     struct bgp_nexthop_cache *bnc, bool specific,
-			     json_object *json)
+			     bool import_check_table, json_object *json)
 {
 	char buf[PREFIX2STR_BUFFER];
 	time_t tbuf;
@@ -977,6 +981,12 @@ static void bgp_show_nexthop(struct vty *vty, struct bgp *bgp,
 	json_object *json_last_update = NULL;
 	json_object *json_nexthop = NULL;
 
+	if (bnc->import_check_table && !import_check_table)
+		return;
+
+	if (bnc->nexthop_check_table && import_check_table)
+		return;
+
 	peer = (struct peer *)bnc->nht_info;
 
 	if (json)
@@ -1103,16 +1113,14 @@ static void bgp_show_nexthops(struct vty *vty, struct bgp *bgp,
 		else
 			vty_out(vty, "Current BGP nexthop cache:\n");
 	}
-	if (import_table)
-		tree = &bgp->import_check_table;
-	else
-		tree = &bgp->nexthop_cache_table;
 
+	tree = &bgp->nexthop_cache_table;
 	if (afi == AFI_IP || afi == AFI_IP6) {
 		if (json)
 			json_afi = json_object_new_object();
 		frr_each (bgp_nexthop_cache, &(*tree)[afi], bnc) {
-			bgp_show_nexthop(vty, bgp, bnc, detail, json_afi);
+			bgp_show_nexthop(vty, bgp, bnc, detail, import_table,
+					 json_afi);
 			found = true;
 		}
 		if (found && json)
@@ -1126,7 +1134,8 @@ static void bgp_show_nexthops(struct vty *vty, struct bgp *bgp,
 		if (json && (afi == AFI_IP || afi == AFI_IP6))
 			json_afi = json_object_new_object();
 		frr_each (bgp_nexthop_cache, &(*tree)[afi], bnc)
-			bgp_show_nexthop(vty, bgp, bnc, detail, json_afi);
+			bgp_show_nexthop(vty, bgp, bnc, detail, import_table,
+					 json_afi);
 		if (json && (afi == AFI_IP || afi == AFI_IP6))
 			json_object_object_add(
 				json, (afi == AFI_IP) ? "ipv4" : "ipv6",
@@ -1162,15 +1171,15 @@ static int show_ip_bgp_nexthop_table(struct vty *vty, const char *name,
 				vty_out(vty, "nexthop address is malformed\n");
 			return CMD_WARNING;
 		}
-		tree = import_table ? &bgp->import_check_table
-				    : &bgp->nexthop_cache_table;
+		tree = &bgp->nexthop_cache_table;
 		if (json)
 			json_afi = json_object_new_object();
 		frr_each (bgp_nexthop_cache, &(*tree)[family2afi(nhop.family)],
 			  bnc) {
 			if (prefix_cmp(&bnc->prefix, &nhop))
 				continue;
-			bgp_show_nexthop(vty, bgp, bnc, true, json_afi);
+			bgp_show_nexthop(vty, bgp, bnc, true, import_table,
+					 json_afi);
 			found = true;
 		}
 		if (json)
@@ -1313,7 +1322,6 @@ void bgp_scan_init(struct bgp *bgp)
 
 	for (afi = AFI_IP; afi < AFI_MAX; afi++) {
 		bgp_nexthop_cache_init(&bgp->nexthop_cache_table[afi]);
-		bgp_nexthop_cache_init(&bgp->import_check_table[afi]);
 		bgp->connected_table[afi] = bgp_table_init(bgp, afi,
 			SAFI_UNICAST);
 	}
@@ -1333,7 +1341,6 @@ void bgp_scan_finish(struct bgp *bgp)
 	for (afi = AFI_IP; afi < AFI_MAX; afi++) {
 		/* Only the current one needs to be reset. */
 		bgp_nexthop_cache_reset(&bgp->nexthop_cache_table[afi]);
-		bgp_nexthop_cache_reset(&bgp->import_check_table[afi]);
 
 		bgp->connected_table[afi]->route_table->cleanup =
 			bgp_connected_cleanup;

diff --git a/bgpd/bgp_nexthop.h b/bgpd/bgp_nexthop.h
@@ -91,6 +91,9 @@ struct bgp_nexthop_cache {
 	 * nexthop.
 	 */
 	bool is_evpn_gwip_nexthop;
+
+	bool import_check_table;
+	bool nexthop_check_table;
 };
 
 extern int bgp_nexthop_cache_compare(const struct bgp_nexthop_cache *a,
@@ -132,8 +135,9 @@ 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,
-					 ifindex_t ifindex);
+					 uint32_t srte_color, ifindex_t ifindex,
+					 bool import_check_table,
+					 bool nexthop_check_table);
 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,

diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c
@@ -378,21 +378,24 @@ int bgp_find_or_add_nexthop(struct bgp *bgp_route, struct bgp *bgp_nexthop,
 	} else
 		return 0;
 
-	if (is_bgp_static_route)
-		tree = &bgp_nexthop->import_check_table[afi];
-	else
-		tree = &bgp_nexthop->nexthop_cache_table[afi];
+	tree = &bgp_nexthop->nexthop_cache_table[afi];
 
 	bnc = bnc_find(tree, &p, srte_color, ifindex);
 	if (!bnc) {
-		bnc = bnc_new(tree, &p, srte_color, ifindex);
+		bnc = bnc_new(tree, &p, srte_color, ifindex,
+			      is_bgp_static_route, !is_bgp_static_route);
 		bnc->bgp = bgp_nexthop;
 		if (BGP_DEBUG(nht, NHT))
 			zlog_debug("Allocated bnc %pFX(%d)(%u)(%s) peer %p",
 				   &bnc->prefix, bnc->ifindex_ipv6_ll,
 				   bnc->srte_color, bnc->bgp->name_pretty,
 				   peer);
 	} else {
+		if (is_bgp_static_route)
+			bnc->import_check_table = true;
+		else
+			bnc->nexthop_check_table = true;
+
 		if (BGP_DEBUG(nht, NHT))
 			zlog_debug(
 				"Found existing bnc %pFX(%d)(%s) flags 0x%x ifindex %d #paths %d peer %p",
@@ -819,12 +822,8 @@ static void bgp_nht_ifp_handle(struct interface *ifp, bool up)
 
 	bgp_nht_ifp_table_handle(bgp, &bgp->nexthop_cache_table[AFI_IP], ifp,
 				 up);
-	bgp_nht_ifp_table_handle(bgp, &bgp->import_check_table[AFI_IP], ifp,
-				 up);
 	bgp_nht_ifp_table_handle(bgp, &bgp->nexthop_cache_table[AFI_IP6], ifp,
 				 up);
-	bgp_nht_ifp_table_handle(bgp, &bgp->import_check_table[AFI_IP6], ifp,
-				 up);
 }
 
 void bgp_nht_ifp_up(struct interface *ifp)
@@ -900,7 +899,7 @@ void bgp_nht_interface_events(struct peer *peer)
 void bgp_parse_nexthop_update(int command, vrf_id_t vrf_id)
 {
 	struct bgp_nexthop_cache_head *tree = NULL;
-	struct bgp_nexthop_cache *bnc_nhc, *bnc_import;
+	struct bgp_nexthop_cache *bnc_nhc;
 	struct bgp *bgp;
 	struct prefix match;
 	struct zapi_route nhr;
@@ -930,19 +929,12 @@ void bgp_parse_nexthop_update(int command, vrf_id_t vrf_id)
 			zlog_debug(
 				"parse nexthop update %pFX(%u)(%s): bnc info not found for nexthop cache",
 				&nhr.prefix, nhr.srte_color, bgp->name_pretty);
-	} else
-		bgp_process_nexthop_update(bnc_nhc, &nhr, false);
-
-	tree = &bgp->import_check_table[afi];
-
-	bnc_import = bnc_find(tree, &match, nhr.srte_color, 0);
-	if (!bnc_import) {
-		if (BGP_DEBUG(nht, NHT))
-			zlog_debug(
-				"parse nexthop update %pFX(%u)(%s): bnc info not found for import check",
-				&nhr.prefix, nhr.srte_color, bgp->name_pretty);
-	} else
-		bgp_process_nexthop_update(bnc_import, &nhr, true);
+	} else {
+		if (bnc_nhc->nexthop_check_table)
+			bgp_process_nexthop_update(bnc_nhc, &nhr, false);
+		if (bnc_nhc->import_check_table)
+			bgp_process_nexthop_update(bnc_nhc, &nhr, true);
+	}
 
 	/*
 	 * HACK: if any BGP route is dependant on an SR-policy that doesn't

diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
@@ -556,9 +556,6 @@ struct bgp {
 	/* Tree for next-hop lookup cache. */
 	struct bgp_nexthop_cache_head nexthop_cache_table[AFI_MAX];
 
-	/* Tree for import-check */
-	struct bgp_nexthop_cache_head import_check_table[AFI_MAX];
-
 	struct bgp_table *connected_table[AFI_MAX];
 
 	struct hash *address_hash;