zebra: enqueue NHG_DEL in rib_nhg meta queue

The NHG_DEL operation is done directly from ZAPI call, whereas the NHG_ADD operation is done in the rib_nhg meta queue. This may be problematic when ADD is followed by DEL. Imagine a scenarion with two protocol NHIDs. <NH1> depends of <NH2> and <NH3>. The deletion of <NH3> at the protocol level will trigger 2 messages to ZEBRA: NHG_ADD(<NH1>) and NHG_DEL(<NH3>). Those operations are properly enqueued in ZAPI, but in the end, the NHG_DEL is executed first. This causes NHG_ADD to unlink an already freed NHG. Fix this by consistently enqueuing NHG_DEL and NHG_ADD operations. Signed-off-by: Philippe Guibert <[email protected]>
pguibert6WIND · Dec 7, 2023 · 4d60d9e · 4d60d9e
1 parent 2648661
commit 4d60d9e
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 23 deletions.
diff --git a/zebra/rib.h b/zebra/rib.h
@@ -430,6 +430,7 @@ extern int rib_queue_nhg_ctx_add(struct nhg_ctx *ctx);
 
 /* Enqueue incoming nhg from proto daemon for processing */
 extern int rib_queue_nhe_add(struct nhg_hash_entry *nhe);
+extern int rib_queue_nhe_del(struct nhg_hash_entry *nhe);
 
 /* Enqueue evpn route for processing */
 int zebra_rib_queue_evpn_route_add(vrf_id_t vrf_id, const struct ethaddr *rmac,

diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c
@@ -1959,20 +1959,19 @@ static void zread_nhg_del(ZAPI_HANDLER_ARGS)
 		return;
 	}
 
-	/*
-	 * Delete the received nhg id
-	 */
-	nhe = zebra_nhg_proto_del(api_nhg.id, api_nhg.proto);
+	/* Create a temporary nhe */
+	nhe = zebra_nhg_alloc();
+	nhe->id = api_nhg.id;
+	nhe->type = api_nhg.proto;
+	nhe->zapi_instance = client->instance;
+	nhe->zapi_session = client->session_id;
+
+	/* Sanity check - Empty nexthop and group */
+	nhe->nhg.nexthop = NULL;
+
+	/* Enqueue to workqueue for processing */
+	rib_queue_nhe_del(nhe);
 
-	if (nhe) {
-		zebra_nhg_decrement_ref(nhe);
-		zsend_nhg_notify(api_nhg.proto, client->instance,
-				 client->session_id, api_nhg.id,
-				 ZAPI_NHG_REMOVED);
-	} else
-		zsend_nhg_notify(api_nhg.proto, client->instance,
-				 client->session_id, api_nhg.id,
-				 ZAPI_NHG_REMOVE_FAIL);
 	/* Stats */
 	client->nhg_del_cnt++;
 }

diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c
@@ -186,6 +186,7 @@ struct wq_nhg_wrapper {
 		struct nhg_ctx *ctx;
 		struct nhg_hash_entry *nhe;
 	} u;
+	bool deletion;
 };
 
 #define WQ_NHG_WRAPPER_TYPE_CTX  0x01
@@ -2531,7 +2532,7 @@ static void process_subq_evpn(struct listnode *lnode)
 static void process_subq_nhg(struct listnode *lnode)
 {
 	struct nhg_ctx *ctx;
-	struct nhg_hash_entry *nhe, *newnhe;
+	struct nhg_hash_entry *nhe, *newnhe, *oldnhe;
 	struct wq_nhg_wrapper *w;
 	uint8_t qindex = META_QUEUE_NHG;
 
@@ -2563,15 +2564,33 @@ static void process_subq_nhg(struct listnode *lnode)
 				   subqueue2str(qindex));
 
 		/* Process incoming nhg update, probably from a proto daemon */
-		newnhe = zebra_nhg_proto_add(nhe->id, nhe->type,
-					     nhe->zapi_instance,
-					     nhe->zapi_session, &nhe->nhg, 0);
+		if (w->deletion) {
+			/*
+			 * Delete the received nhg id
+			 */
+			oldnhe = zebra_nhg_proto_del(nhe->id, nhe->type);
+			if (oldnhe) {
+				zsend_nhg_notify(nhe->type, nhe->zapi_instance,
+						 nhe->zapi_session, nhe->id,
+						 ZAPI_NHG_REMOVED);
+				zebra_nhg_decrement_ref(oldnhe);
+			} else
+				zsend_nhg_notify(nhe->type, nhe->zapi_instance,
+						 nhe->zapi_session, nhe->id,
+						 ZAPI_NHG_REMOVE_FAIL);
 
-		/* Report error to daemon via ZAPI */
-		if (newnhe == NULL)
-			zsend_nhg_notify(nhe->type, nhe->zapi_instance,
-					 nhe->zapi_session, nhe->id,
-					 ZAPI_NHG_FAIL_INSTALL);
+		} else {
+			newnhe = zebra_nhg_proto_add(nhe->id, nhe->type,
+						     nhe->zapi_instance,
+						     nhe->zapi_session,
+						     &nhe->nhg, 0);
+
+			/* Report error to daemon via ZAPI */
+			if (newnhe == NULL)
+				zsend_nhg_notify(nhe->type, nhe->zapi_instance,
+						 nhe->zapi_session, nhe->id,
+						 ZAPI_NHG_FAIL_INSTALL);
+		}
 
 		/* Free temp nhe - we own that memory. */
 		zebra_nhg_free(nhe);
@@ -3339,7 +3358,8 @@ static int rib_meta_queue_nhg_ctx_add(struct meta_queue *mq, void *data)
 	return 0;
 }
 
-static int rib_meta_queue_nhg_add(struct meta_queue *mq, void *data)
+static int rib_meta_queue_nhg_process(struct meta_queue *mq, void *data,
+				      bool deletion)
 {
 	struct nhg_hash_entry *nhe = NULL;
 	uint8_t qindex = META_QUEUE_NHG;
@@ -3354,6 +3374,7 @@ static int rib_meta_queue_nhg_add(struct meta_queue *mq, void *data)
 
 	w->type = WQ_NHG_WRAPPER_TYPE_NHG;
 	w->u.nhe = nhe;
+	w->deletion = deletion;
 
 	listnode_add(mq->subq[qindex], w);
 	mq->size++;
@@ -3365,6 +3386,16 @@ static int rib_meta_queue_nhg_add(struct meta_queue *mq, void *data)
 	return 0;
 }
 
+static int rib_meta_queue_nhg_add(struct meta_queue *mq, void *data)
+{
+	return rib_meta_queue_nhg_process(mq, data, false);
+}
+
+static int rib_meta_queue_nhg_del(struct meta_queue *mq, void *data)
+{
+	return rib_meta_queue_nhg_process(mq, data, true);
+}
+
 static int rib_meta_queue_evpn_add(struct meta_queue *mq, void *data)
 {
 	listnode_add(mq->subq[META_QUEUE_EVPN], data);
@@ -3472,6 +3503,17 @@ int rib_queue_nhe_add(struct nhg_hash_entry *nhe)
 	return mq_add_handler(nhe, rib_meta_queue_nhg_add);
 }
 
+/*
+ * Enqueue incoming nhg from proto daemon for processing
+ */
+int rib_queue_nhe_del(struct nhg_hash_entry *nhe)
+{
+	if (nhe == NULL)
+		return -1;
+
+	return mq_add_handler(nhe, rib_meta_queue_nhg_del);
+}
+
 /*
  * Enqueue evpn route for processing
  */