bgpd: dynamically allocate synchronization primitives

Changes all synchronization primitives to be dynamically allocated. This
should help catch any subtle errors in pthread lifecycles.

This change also pre-initializes synchronization primitives before
threads begin to run, eliminating a potential race condition that
probably would have caused a segfault on startup on a very fast box.

Also changes mutex and condition variable allocations to use
MTYPE_PTHREAD and updates tests to do the proper initializations.

Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>

bgpd/bgp_keepalives.c

@@ -48,12 +48,12 @@ struct pkat {
 };
 
 /* List of peers we are sending keepalives for, and associated mutex. */
-static pthread_mutex_t peerlist_mtx = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t *peerlist_mtx;
-static pthread_cond_t peerlist_cond;
+static pthread_cond_t *peerlist_cond;
 static struct list *peerlist;
 
 /* Thread control flag. */
-bool bgp_keepalives_thread_run;
+bool bgp_keepalives_thread_run = false;
 
 static struct pkat *pkat_new(struct peer *peer)
 {
@@ -67,22 +67,6 @@ static void pkat_del(void *pkat)
 {
 	XFREE(MTYPE_TMP, pkat);
 }
-/**
- * Cleanup thread resources at termination.
- *
- * @param arg not used
- */
-static void cleanup_handler(void *arg)
-{
-	if (peerlist)
-		list_delete(peerlist);
-
-	peerlist = NULL;
-
-	pthread_mutex_unlock(&peerlist_mtx);
-	pthread_cond_destroy(&peerlist_cond);
-	memset(&peerlist_cond, 0, sizeof(peerlist_cond));
-}
 
 /*
  * Walks the list of peers, sending keepalives to those that are due for them.
@@ -151,39 +135,69 @@ static struct timeval update()
 	return next_update;
 }
 
+void peer_keepalives_init()
+{
+	peerlist_mtx = XCALLOC(MTYPE_PTHREAD, sizeof(pthread_mutex_t));
+	peerlist_cond = XCALLOC(MTYPE_PTHREAD, sizeof(pthread_cond_t));
+
+	// initialize mutex
+	pthread_mutex_init(peerlist_mtx, NULL);
+
+	// use monotonic clock with condition variable
+	pthread_condattr_t attrs;
+	pthread_condattr_init(&attrs);
+	pthread_condattr_setclock(&attrs, CLOCK_MONOTONIC);
+	pthread_cond_init(peerlist_cond, &attrs);
+	pthread_condattr_destroy(&attrs);
+
+	// initialize peerlist
+	peerlist = list_new();
+	peerlist->del = pkat_del;
+}
+
+static void peer_keepalives_finish(void *arg)
+{
+	bgp_keepalives_thread_run = false;
+
+	if (peerlist)
+		list_delete(peerlist);
+
+	peerlist = NULL;
+
+	pthread_mutex_unlock(peerlist_mtx);
+	pthread_mutex_destroy(peerlist_mtx);
+	pthread_cond_destroy(peerlist_cond);
+
+	XFREE(MTYPE_PTHREAD, peerlist_mtx);
+	XFREE(MTYPE_PTHREAD, peerlist_cond);
+}
+
+/**
+ * Entry function for peer keepalive generation pthread.
+ *
+ * peer_keepalives_init() must be called prior to this.
+ */
 void *peer_keepalives_start(void *arg)
 {
 	struct timeval currtime = {0, 0};
 	struct timeval next_update = {0, 0};
 	struct timespec next_update_ts = {0, 0};
 
-	// initialize synchronization primitives
+	pthread_mutex_lock(peerlist_mtx);
-	pthread_mutex_lock(&peerlist_mtx);
 
-	// use monotonic clock with condition variable
+	// register cleanup handler
-	pthread_condattr_t attrs;
+	pthread_cleanup_push(&peer_keepalives_finish, NULL);
-	pthread_condattr_init(&attrs);
-	pthread_condattr_setclock(&attrs, CLOCK_MONOTONIC);
-	pthread_cond_init(&peerlist_cond, &attrs);
-
-	// initialize peerlist
-	peerlist = list_new();
-	peerlist->del = pkat_del;
-
-	// register cleanup handlers
-	pthread_cleanup_push(&cleanup_handler, NULL);
 
 	bgp_keepalives_thread_run = true;
 
 	while (bgp_keepalives_thread_run) {
 		if (peerlist->count > 0)
-			pthread_cond_timedwait(&peerlist_cond, &peerlist_mtx,
+			pthread_cond_timedwait(peerlist_cond, peerlist_mtx,
 					       &next_update_ts);
 		else
 			while (peerlist->count == 0
 			       && bgp_keepalives_thread_run)
-				pthread_cond_wait(&peerlist_cond,
+				pthread_cond_wait(peerlist_cond, peerlist_mtx);
-						  &peerlist_mtx);
 
 		monotime(&currtime);
 		next_update = update();
@@ -201,14 +215,14 @@ void *peer_keepalives_start(void *arg)
 
 void peer_keepalives_on(struct peer *peer)
 {
-	pthread_mutex_lock(&peerlist_mtx);
+	pthread_mutex_lock(peerlist_mtx);
 	{
 		struct listnode *ln, *nn;
 		struct pkat *pkat;
 
 		for (ALL_LIST_ELEMENTS(peerlist, ln, nn, pkat))
 			if (pkat->peer == peer) {
-				pthread_mutex_unlock(&peerlist_mtx);
+				pthread_mutex_unlock(peerlist_mtx);
 				return;
 			}
 
@@ -217,13 +231,13 @@ void peer_keepalives_on(struct peer *peer)
 		peer_lock(peer);
 		SET_FLAG(peer->thread_flags, PEER_THREAD_KEEPALIVES_ON);
 	}
-	pthread_mutex_unlock(&peerlist_mtx);
+	pthread_mutex_unlock(peerlist_mtx);
 	peer_keepalives_wake();
 }
 
 void peer_keepalives_off(struct peer *peer)
 {
-	pthread_mutex_lock(&peerlist_mtx);
+	pthread_mutex_lock(peerlist_mtx);
 	{
 		struct listnode *ln, *nn;
 		struct pkat *pkat;
@@ -237,14 +251,14 @@ void peer_keepalives_off(struct peer *peer)
 
 		UNSET_FLAG(peer->thread_flags, PEER_THREAD_KEEPALIVES_ON);
 	}
-	pthread_mutex_unlock(&peerlist_mtx);
+	pthread_mutex_unlock(peerlist_mtx);
 }
 
 void peer_keepalives_wake()
 {
-	pthread_mutex_lock(&peerlist_mtx);
+	pthread_mutex_lock(peerlist_mtx);
 	{
-		pthread_cond_signal(&peerlist_cond);
+		pthread_cond_signal(peerlist_cond);
 	}
-	pthread_mutex_unlock(&peerlist_mtx);
+	pthread_mutex_unlock(peerlist_mtx);
 }

bgpd/bgp_keepalives.h

@@ -58,6 +58,13 @@ extern void peer_keepalives_on(struct peer *);
  */
 extern void peer_keepalives_off(struct peer *);
 
+/* Pre-run initialization function for keepalives pthread.
+ *
+ * Initializes synchronization primitives. This should be called before
+ * anything else to avoid race conditions.
+ */
+extern void peer_keepalives_init(void);
+
 /* Entry function for keepalives pthread.
  *
  * This function loops over an internal list of peers, generating keepalives at

bgpd/bgp_main.c

@@ -400,7 +400,7 @@ int main(int argc, char **argv)
 
 	frr_config_fork();
 	/* must be called after fork() */
-	bgp_pthreads_init();
+	bgp_pthreads_run();
 	frr_run(bm->master);
 
 	/* Not reached. */

bgpd/bgp_packet.c

@@ -57,14 +57,14 @@
 #include "bgpd/bgp_label.h"
 
 /* Linked list of active peers */
-static pthread_mutex_t plist_mtx = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t *plist_mtx;
-static pthread_cond_t write_cond = PTHREAD_COND_INITIALIZER;
+static pthread_cond_t *write_cond;
 static struct list *plist;
 
 /* periodically scheduled thread to generate update-group updates */
 static struct thread *t_generate_updgrp_packets;
 
-bool bgp_packet_writes_thread_run;
+bool bgp_packet_writes_thread_run = false;
 
 /* Set up BGP packet marker and packet type. */
 int bgp_packet_set_marker(struct stream *s, u_char type)
@@ -271,7 +271,7 @@ static int bgp_generate_updgrp_packets(struct thread *thread)
 {
 	struct listnode *ln;
 	struct peer *peer;
-	pthread_mutex_lock(&plist_mtx);
+	pthread_mutex_lock(plist_mtx);
 	{
 		for (ALL_LIST_ELEMENTS_RO(plist, ln, peer))
 			while (bgp_write_packet(peer))
@@ -279,7 +279,7 @@ static int bgp_generate_updgrp_packets(struct thread *thread)
 
 		t_generate_updgrp_packets = NULL;
 	}
-	pthread_mutex_unlock(&plist_mtx);
+	pthread_mutex_unlock(plist_mtx);
 	return 0;
 }
 
@@ -2270,20 +2270,46 @@ done : {
 	return count;
 }
 
-static void cleanup_handler(void *arg)
+void peer_writes_init(void)
 {
+	plist_mtx = XCALLOC(MTYPE_PTHREAD, sizeof(pthread_mutex_t));
+	write_cond = XCALLOC(MTYPE_PTHREAD, sizeof(pthread_cond_t));
+
+	// initialize mutex
+	pthread_mutex_init(plist_mtx, NULL);
+
+	// use monotonic clock with condition variable
+	pthread_condattr_t attrs;
+	pthread_condattr_init(&attrs);
+	pthread_condattr_setclock(&attrs, CLOCK_MONOTONIC);
+	pthread_cond_init(write_cond, &attrs);
+	pthread_condattr_destroy(&attrs);
+
+	// initialize peerlist
+	plist = list_new();
+}
+
+static void peer_writes_finish(void *arg)
+{
+	bgp_packet_writes_thread_run = false;
+
 	if (plist)
 		list_delete(plist);
 
 	plist = NULL;
 
-	pthread_mutex_unlock(&plist_mtx);
+	pthread_mutex_unlock(plist_mtx);
+	pthread_mutex_destroy(plist_mtx);
+	pthread_cond_destroy(write_cond);
+
+	XFREE(MTYPE_PTHREAD, plist_mtx);
+	XFREE(MTYPE_PTHREAD, write_cond);
 }
 
 /**
  * Entry function for peer packet flushing pthread.
  *
- * The plist must be initialized before calling this.
+ * peer_writes_init() must be called prior to this.
  */
 void *peer_writes_start(void *arg)
 {
@@ -2291,26 +2317,25 @@ void *peer_writes_start(void *arg)
 	struct timeval sleeptime = {0, 500};
 	struct timespec next_update = {0, 0};
 
-	// initialize
-	pthread_mutex_lock(&plist_mtx);
-	plist = list_new();
-
 	struct listnode *ln;
 	struct peer *peer;
 
-	pthread_cleanup_push(&cleanup_handler, NULL);
+	pthread_mutex_lock(plist_mtx);
+
+	// register cleanup handler
+	pthread_cleanup_push(&peer_writes_finish, NULL);
 
 	bgp_packet_writes_thread_run = true;
 
 	while (bgp_packet_writes_thread_run) { // wait around until next update
 					       // time
 		if (plist->count > 0)
-			pthread_cond_timedwait(&write_cond, &plist_mtx,
+			pthread_cond_timedwait(write_cond, plist_mtx,
 					       &next_update);
 		else // wait around until we have some peers
 			while (plist->count == 0
 			       && bgp_packet_writes_thread_run)
-				pthread_cond_wait(&write_cond, &plist_mtx);
+				pthread_cond_wait(write_cond, plist_mtx);
 
 		for (ALL_LIST_ELEMENTS_RO(plist, ln, peer)) {
 			pthread_mutex_lock(&peer->obuf_mtx);
@@ -2329,7 +2354,7 @@ void *peer_writes_start(void *arg)
 				bm->master, bgp_generate_updgrp_packets, NULL,
 				0);
 
-		gettimeofday(&currtime, NULL);
+		monotime(&currtime);
 		timeradd(&currtime, &sleeptime, &currtime);
 		TIMEVAL_TO_TIMESPEC(&currtime, &next_update);
 	}
@@ -2348,7 +2373,7 @@ void peer_writes_on(struct peer *peer)
 	if (peer->status == Deleted)
 		return;
 
-	pthread_mutex_lock(&plist_mtx);
+	pthread_mutex_lock(plist_mtx);
 	{
 		struct listnode *ln, *nn;
 		struct peer *p;
@@ -2356,7 +2381,7 @@ void peer_writes_on(struct peer *peer)
 		// make sure this peer isn't already in the list
 		for (ALL_LIST_ELEMENTS(plist, ln, nn, p))
 			if (p == peer) {
-				pthread_mutex_unlock(&plist_mtx);
+				pthread_mutex_unlock(plist_mtx);
 				return;
 			}
 
@@ -2365,7 +2390,7 @@ void peer_writes_on(struct peer *peer)
 
 		SET_FLAG(peer->thread_flags, PEER_THREAD_WRITES_ON);
 	}
-	pthread_mutex_unlock(&plist_mtx);
+	pthread_mutex_unlock(plist_mtx);
 	peer_writes_wake();
 }
 
@@ -2376,7 +2401,7 @@ void peer_writes_off(struct peer *peer)
 {
 	struct listnode *ln, *nn;
 	struct peer *p;
-	pthread_mutex_lock(&plist_mtx);
+	pthread_mutex_lock(plist_mtx);
 	{
 		for (ALL_LIST_ELEMENTS(plist, ln, nn, p))
 			if (p == peer) {
@@ -2387,7 +2412,7 @@ void peer_writes_off(struct peer *peer)
 
 		UNSET_FLAG(peer->thread_flags, PEER_THREAD_WRITES_ON);
 	}
-	pthread_mutex_unlock(&plist_mtx);
+	pthread_mutex_unlock(plist_mtx);
 }
 
 /**
@@ -2395,5 +2420,5 @@ void peer_writes_off(struct peer *peer)
  */
 void peer_writes_wake()
 {
-	pthread_cond_signal(&write_cond);
+	pthread_cond_signal(write_cond);
 }

bgpd/bgp_packet.h

@@ -67,6 +67,7 @@ extern int bgp_packet_set_size(struct stream *s);
 /* Control variable for write thread. */
 extern bool bgp_packet_writes_thread_run;
 
+extern void peer_writes_init(void);
 extern void *peer_writes_start(void *arg);
 extern void peer_writes_on(struct peer *peer);
 extern void peer_writes_off(struct peer *peer);

bgpd/bgpd.c

@@ -7387,7 +7387,14 @@ static const struct cmd_variable_handler bgp_viewvrf_var_handlers[] = {
 
 void bgp_pthreads_init()
 {
-	/* init write & keepalive threads */
+	/* pre-run initialization */
+	peer_keepalives_init();
+	peer_writes_init();
+}
+
+void bgp_pthreads_run()
+{
+	/* run threads */
 	pthread_create(bm->t_bgp_keepalives, NULL, &peer_keepalives_start,
 		       NULL);
 	pthread_create(bm->t_bgp_packet_writes, NULL, &peer_writes_start, NULL);
@@ -7417,6 +7424,9 @@ void bgp_init(void)
 	/* allocates some vital data structures used by peer commands in
 	 * vty_init */
 
+	/* pre-init pthreads */
+	bgp_pthreads_init();
+
 	/* Init zebra. */
 	bgp_zebra_init(bm->master);
 

bgpd/bgpd.h

@@ -1260,7 +1260,7 @@ extern int bgp_config_write(struct vty *);
 extern void bgp_master_init(struct thread_master *master);
 
 extern void bgp_init(void);
-extern void bgp_pthreads_init(void);
+extern void bgp_pthreads_run(void);
 extern void bgp_pthreads_finish(void);
 extern void bgp_route_map_init(void);
 extern void bgp_session_reset(struct peer *);

tests/bgpd/test_aspath.c

@@ -29,6 +29,7 @@
 #include "bgpd/bgpd.h"
 #include "bgpd/bgp_aspath.h"
 #include "bgpd/bgp_attr.h"
+#include "bgpd/bgp_packet.h"
 
 #define VT100_RESET "\x1b[0m"
 #define VT100_RED "\x1b[31m"
@@ -1341,6 +1342,7 @@ int main(void)
 	master = bm->master;
 	bgp_option_set(BGP_OPT_NO_LISTEN);
 	bgp_attr_init();
+	peer_writes_init();
 
 	while (test_segments[i].name) {
 		printf("test %u\n", i);

tests/bgpd/test_capability.c

@@ -903,6 +903,7 @@ int main(void)
 	bgp_master_init(master);
 	vrf_init(NULL, NULL, NULL, NULL);
 	bgp_option_set(BGP_OPT_NO_LISTEN);
+	peer_writes_init();
 
 	if (fileno(stdout) >= 0)
 		tty = isatty(fileno(stdout));