From: Stephen Worley <sworley@cumulusnetworks.com>
Date: Wed, 20 May 2020 19:47:12 +0000 (-0400)
Subject: zebra: fix refcnt/rib issues in NHG replace/delete
X-Git-Tag: base_7.6~489^2~30
X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=1f655680469dc883298348b2bdd4c2ae8bff0fe7;p=matthieu%2Ffrr.git

zebra: fix refcnt/rib issues in NHG replace/delete

Fix some reference counting issues seen when replacing
a NHG and deleting one.

For replacement, we should end with the same refcnt on the new
one.

For delete, its the caller's job to decrement its ref after
its done with it.

Further, update routes in the rib with the new pointer after replace.

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
---

diff --git a/zebra/rib.h b/zebra/rib.h
index be680a112f..9ddc35b091 100644
--- a/zebra/rib.h
+++ b/zebra/rib.h
@@ -333,6 +333,10 @@ extern void route_entry_copy_nexthops(struct route_entry *re,
 int route_entry_update_nhe(struct route_entry *re,
 			   struct nhg_hash_entry *new_nhghe);
 
+/* NHG replace has happend, we have to update route_entry pointers to new one */
+void rib_handle_nhg_replace(struct nhg_hash_entry *old,
+			    struct nhg_hash_entry *new);
+
 #define route_entry_dump(prefix, src, re) _route_entry_dump(__func__, prefix, src, re)
 extern void _route_entry_dump(const char *func, union prefixconstptr pp,
 			      union prefixconstptr src_pp,
diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c
index 4cd733ea35..c8220af81e 100644
--- a/zebra/zapi_msg.c
+++ b/zebra/zapi_msg.c
@@ -1825,8 +1825,6 @@ static void zread_nhg_reader(ZAPI_HANDLER_ARGS)
 	 * Resolution is going to need some more work.
 	 */
 	if (nhe) {
-		zebra_nhg_increment_ref(nhe);
-		zebra_nhg_install_kernel(nhe);
 		nhg_notify(proto, client->instance, id, ZAPI_NHG_INSTALLED);
 	} else
 		nhg_notify(proto, client->instance, id, ZAPI_NHG_FAIL_INSTALL);
diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c
index 407af2902d..1c9b970008 100644
--- a/zebra/zebra_nhg.c
+++ b/zebra/zebra_nhg.c
@@ -2741,6 +2741,26 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type,
 	struct nhg_hash_entry lookup;
 	struct nhg_hash_entry *new, *old;
 	struct nhg_connected *rb_node_dep = NULL;
+	struct nexthop *newhop;
+	bool replace = false;
+
+	if (!nhg->nexthop) {
+		if (IS_ZEBRA_DEBUG_NHG)
+			zlog_debug("%s: id %u, no nexthops passed to add",
+				   __func__, id);
+		return NULL;
+	}
+
+
+	/* Set nexthop list as active, since they wont go through rib
+	 * processing.
+	 *
+	 * Assuming valid/onlink for now.
+	 *
+	 * Once resolution is figured out, we won't need this!
+	 */
+	for (ALL_NEXTHOPS_PTR(nhg, newhop))
+		SET_FLAG(newhop->flags, NEXTHOP_FLAG_ACTIVE);
 
 	zebra_nhe_init(&lookup, afi, nhg->nexthop);
 	lookup.nhg.nexthop = nhg->nexthop;
@@ -2754,36 +2774,51 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type,
 		 * This is a replace, just release NHE from ID for now, The
 		 * depends/dependents may still be used in the replacement.
 		 */
+		replace = true;
 		hash_release(zrouter.nhgs_id, old);
 	}
 
 	new = zebra_nhg_rib_find_nhe(&lookup, afi);
 
+	zebra_nhg_increment_ref(new);
+
+	zebra_nhg_set_valid_if_active(new);
+
+	zebra_nhg_install_kernel(new);
+
 	if (old) {
-		/* Now release depends/dependents in old one */
-		zebra_nhg_release_all_deps(old);
-	}
+		rib_handle_nhg_replace(old, new);
 
-	if (!new)
-		return NULL;
+		/* if this != 1 at this point, we have a bug */
+		assert(old->refcnt == 1);
 
-	/* TODO: Assuming valid/onlink for now */
-	SET_FLAG(new->flags, NEXTHOP_GROUP_VALID);
+		/* We have to decrement its singletons
+		 * because some might not exist in NEW.
+		 */
+		if (!zebra_nhg_depends_is_empty(old)) {
+			frr_each (nhg_connected_tree, &old->nhg_depends,
+				  rb_node_dep)
+				zebra_nhg_decrement_ref(rb_node_dep->nhe);
+		}
+
+		/* Free all the things */
+		zebra_nhg_release_all_deps(old);
 
-	if (!zebra_nhg_depends_is_empty(new)) {
-		frr_each (nhg_connected_tree, &new->nhg_depends, rb_node_dep)
-			SET_FLAG(rb_node_dep->nhe->flags, NEXTHOP_GROUP_VALID);
+		/* Dont call the dec API, we dont want to uninstall the ID */
+		old->refcnt = 0;
+		zebra_nhg_free(old);
+		old = NULL;
 	}
 
 	if (IS_ZEBRA_DEBUG_NHG_DETAIL)
 		zlog_debug("%s: %s nhe %p (%u), vrf %d, type %s", __func__,
-			   (old ? "replaced" : "added"), new, new->id,
+			   (replace ? "replaced" : "added"), new, new->id,
 			   new->vrf_id, zebra_route_string(new->type));
 
 	return new;
 }
 
-/* Delete NHE from upper level proto */
+/* Delete NHE from upper level proto, caller must decrement ref */
 struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id)
 {
 	struct nhg_hash_entry *nhe;
@@ -2797,11 +2832,12 @@ struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id)
 		return NULL;
 	}
 
-	if (nhe->refcnt) {
+	if (nhe->refcnt > 1) {
 		/* TODO: should be warn? */
 		if (IS_ZEBRA_DEBUG_NHG)
-			zlog_debug("%s: id %u, still being used refcnt %u",
-				   __func__, nhe->id, nhe->refcnt);
+			zlog_debug(
+				"%s: id %u, still being used by routes refcnt %u",
+				__func__, nhe->id, nhe->refcnt);
 		return NULL;
 	}
 
diff --git a/zebra/zebra_nhg.h b/zebra/zebra_nhg.h
index 8c33a91917..6c9f20d0a4 100644
--- a/zebra/zebra_nhg.h
+++ b/zebra/zebra_nhg.h
@@ -281,7 +281,7 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type,
  *
  * Returns deleted NHE on success, otherwise NULL.
  *
- * Caller must free the NHE.
+ * Caller must decrement ref with zebra_nhg_decrement_ref() when done.
  */
 struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id);
 
diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c
index 5a7967d817..aac0e628fe 100644
--- a/zebra/zebra_rib.c
+++ b/zebra/zebra_rib.c
@@ -235,7 +235,7 @@ int route_entry_update_nhe(struct route_entry *re,
 		goto done;
 	}
 
-	if ((re->nhe_id != 0) && (re->nhe_id != new_nhghe->id)) {
+	if ((re->nhe_id != 0) && re->nhe && (re->nhe != new_nhghe)) {
 		old = re->nhe;
 
 		route_entry_attach_ref(re, new_nhghe);
@@ -250,6 +250,29 @@ done:
 	return ret;
 }
 
+void rib_handle_nhg_replace(struct nhg_hash_entry *old,
+			    struct nhg_hash_entry *new)
+{
+	struct zebra_router_table *zrt;
+	struct route_node *rn;
+	struct route_entry *re, *next;
+
+	if (IS_ZEBRA_DEBUG_RIB_DETAILED || IS_ZEBRA_DEBUG_NHG_DETAIL)
+		zlog_debug("%s: replacing routes nhe (%u) OLD %p NEW %p",
+			   __func__, new->id, new, old);
+
+	/* We have to do them ALL */
+	RB_FOREACH (zrt, zebra_router_table_head, &zrouter.tables) {
+		for (rn = route_top(zrt->table); rn;
+		     rn = srcdest_route_next(rn)) {
+			RNODE_FOREACH_RE_SAFE (rn, re, next) {
+				if (re->nhe && re->nhe == old)
+					route_entry_update_nhe(re, new);
+			}
+		}
+	}
+}
+
 struct route_entry *rib_match(afi_t afi, safi_t safi, vrf_id_t vrf_id,
 			      union g_addr *addr, struct route_node **rn_out)
 {