Merge pull request #4550 from manuhalo/fix_bgp_label_cb

Fix bgp-lu label callback crash

bgpd/bgp_label.c

@@ -130,10 +130,21 @@ mpls_label_t bgp_adv_label(struct bgp_node *rn, struct bgp_path_info *pi,
 int bgp_reg_for_label_callback(mpls_label_t new_label, void *labelid,
 			       bool allocated)
 {
-	struct bgp_path_info *pi = (struct bgp_path_info *)labelid;
-	struct bgp_node *rn = (struct bgp_node *)pi->net;
+	struct bgp_path_info *pi;
+	struct bgp_node *rn;
 	char addr[PREFIX_STRLEN];
 
+	pi = labelid;
+	/* Is this path still valid? */
+	if (!bgp_path_info_unlock(pi)) {
+		if (BGP_DEBUG(labelpool, LABELPOOL))
+			zlog_debug(
+				"%s: bgp_path_info is no longer valid, ignoring",
+				__func__);
+		return -1;
+	}
+
+	rn = pi->net;
 	prefix2str(&rn->p, addr, PREFIX_STRLEN);
 
 	if (BGP_DEBUG(labelpool, LABELPOOL))

bgpd/bgp_labelpool.c

@@ -180,9 +180,24 @@ void bgp_lp_init(struct thread_master *master, struct labelpool *pool)
 	lp->callback_q->spec.max_retries = 0;
 }
 
+/* check if a label callback was for a BGP LU path, and if so, unlock it */
+static void check_bgp_lu_cb_unlock(struct lp_lcb *lcb)
+{
+	if (lcb->type == LP_TYPE_BGP_LU)
+		bgp_path_info_unlock(lcb->labelid);
+}
+
+/* check if a label callback was for a BGP LU path, and if so, lock it */
+static void check_bgp_lu_cb_lock(struct lp_lcb *lcb)
+{
+	if (lcb->type == LP_TYPE_BGP_LU)
+		bgp_path_info_lock(lcb->labelid);
+}
+
 void bgp_lp_finish(void)
 {
 	struct lp_fifo *lf;
+	struct work_queue_item *item, *titem;
 
 	if (!lp)
 		return;
@@ -195,10 +210,21 @@ void bgp_lp_finish(void)
 
 	list_delete(&lp->chunks);
 
-	while ((lf = lp_fifo_pop(&lp->requests)))
+	while ((lf = lp_fifo_pop(&lp->requests))) {
+		check_bgp_lu_cb_unlock(&lf->lcb);
 		XFREE(MTYPE_BGP_LABEL_FIFO, lf);
+	}
 	lp_fifo_fini(&lp->requests);
 
+	/* we must unlock path infos for LU callbacks; but we cannot do that
+	 * in the deletion callback of the workqueue, as that is also called
+	 * to remove an element from the queue after it has been run, resulting
+	 * in a double unlock. Hence we need to iterate over our queues and
+	 * lists and manually perform the unlocking (ugh)
+	 */
+	STAILQ_FOREACH_SAFE (item, &lp->callback_q->items, wq, titem)
+		check_bgp_lu_cb_unlock(item->data);
+
 	work_queue_free_and_null(&lp->callback_q);
 
 	lp = NULL;
@@ -328,6 +354,9 @@ void bgp_lp_get(
 		q->labelid = lcb->labelid;
 		q->allocated = true;
 
+		/* if this is a LU request, lock path info before queueing */
+		check_bgp_lu_cb_lock(lcb);
+
 		work_queue_add(lp->callback_q, q);
 
 		return;
@@ -353,13 +382,16 @@ void bgp_lp_get(
 		sizeof(struct lp_fifo));
 
 	lf->lcb = *lcb;
+	/* if this is a LU request, lock path info before queueing */
+	check_bgp_lu_cb_lock(lcb);
+
 	lp_fifo_add_tail(&lp->requests, lf);
 
 	if (lp_fifo_count(&lp->requests) > lp->pending_count) {
-		if (!zclient_send_get_label_chunk(zclient, 0, LP_CHUNK_SIZE)) {
-			lp->pending_count += LP_CHUNK_SIZE;
+		if (!zclient || zclient->sock < 0)
 			return;
-		}
+		if (!zclient_send_get_label_chunk(zclient, 0, LP_CHUNK_SIZE))
+			lp->pending_count += LP_CHUNK_SIZE;
 	}
 }
 
@@ -436,6 +468,10 @@ void bgp_lp_event_chunk(uint8_t keep, uint32_t first, uint32_t last)
 						__func__, labelid,
 						lcb->label, lcb->label, lcb);
 			}
+			/* if this was a BGP_LU request, unlock path info node
+			 */
+			check_bgp_lu_cb_unlock(lcb);
+
 			goto finishedrequest;
 		}
 
@@ -510,8 +546,10 @@ void bgp_lp_event_zebra_up(void)
 
 	lm_init_ok = lm_label_manager_connect(zclient, 1) == 0;
 
-	if (!lm_init_ok)
+	if (!lm_init_ok) {
 		zlog_err("%s: label manager connection error", __func__);
+		return;
+	}
 
 	zclient_send_get_label_chunk(zclient, 0, labels_needed);
 	lp->pending_count = labels_needed;
@@ -544,6 +582,7 @@ void bgp_lp_event_zebra_up(void)
 				q->label = lcb->label;
 				q->labelid = lcb->labelid;
 				q->allocated = false;
+				check_bgp_lu_cb_lock(lcb);
 				work_queue_add(lp->callback_q, q);
 
 				lcb->label = MPLS_LABEL_NONE;
@@ -556,6 +595,7 @@ void bgp_lp_event_zebra_up(void)
 				sizeof(struct lp_fifo));
 
 			lf->lcb = *lcb;
+			check_bgp_lu_cb_lock(lcb);
 			lp_fifo_add_tail(&lp->requests, lf);
 		}
 

zebra/label_manager.c

@@ -75,6 +75,10 @@ static int relay_response_back(void)
 	unsigned short instance;
 	struct zserv *zserv;
 
+	/* sanity */
+	if (!zclient || zclient->sock < 0)
+		return -1;
+
 	/* input buffer with msg from label manager */
 	src = zclient->ibuf;
 
@@ -83,10 +87,11 @@ static int relay_response_back(void)
 	/* parse header */
 	ret = zclient_read_header(src, zclient->sock, &size, &marker, &version,
 				  &vrf_id, &resp_cmd);
-	if (ret < 0 && errno != EAGAIN) {
-		flog_err(EC_ZEBRA_LM_RESPONSE,
-			 "Error reading Label Manager response: %s",
-			 strerror(errno));
+	if (ret < 0) {
+		if (errno != EAGAIN)
+			flog_err(EC_ZEBRA_LM_RESPONSE,
+				 "Error reading Label Manager response: %s",
+				 strerror(errno));
 		return -1;
 	}