From ea63ff6bbd15e1f105fedee071ba3917daa41c7a Mon Sep 17 00:00:00 2001 From: Emanuele Di Pascale Date: Tue, 18 Jun 2019 11:14:28 +0200 Subject: [PATCH] bgpd: fix LU label callback crash under some conditions, the callback to get a label for a LU bgp path could be called after the path had already been freed. In this case we would be reading garbage and potentially crash. Lock the path info before queueing the callback, and unlock as the first step of the callback, exiting gracefully if the path info is now NULL. Signed-off-by: Emanuele Di Pascale --- bgpd/bgp_label.c | 15 +++++++++++-- bgpd/bgp_labelpool.c | 50 +++++++++++++++++++++++++++++++++++++++----- 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/bgpd/bgp_label.c b/bgpd/bgp_label.c index 9511650842..489ac6ea9f 100644 --- a/bgpd/bgp_label.c +++ b/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)) diff --git a/bgpd/bgp_labelpool.c b/bgpd/bgp_labelpool.c index 71c0c8c7c6..e0e3d7ff2e 100644 --- a/bgpd/bgp_labelpool.c +++ b/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); } -- 2.39.5