From 419dfe6a7049c0ee7f469cc10724ea110bef3c9c Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 29 Mar 2017 19:16:28 +0000 Subject: [PATCH] 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 --- bgpd/bgp_keepalives.c | 92 +++++++++++++++++++++--------------- bgpd/bgp_keepalives.h | 7 +++ bgpd/bgp_main.c | 2 +- bgpd/bgp_packet.c | 69 ++++++++++++++++++--------- bgpd/bgp_packet.h | 1 + bgpd/bgpd.c | 12 ++++- bgpd/bgpd.h | 2 +- tests/bgpd/test_aspath.c | 2 + tests/bgpd/test_capability.c | 1 + 9 files changed, 124 insertions(+), 64 deletions(-) diff --git a/bgpd/bgp_keepalives.c b/bgpd/bgp_keepalives.c index 4944e3ea23..0d497e0aec 100644 --- a/bgpd/bgp_keepalives.c +++ b/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_cond_t peerlist_cond; +static pthread_mutex_t *peerlist_mtx; +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_start(void *arg) +void peer_keepalives_init() { - struct timeval currtime = {0, 0}; - struct timeval next_update = {0, 0}; - struct timespec next_update_ts = {0, 0}; + peerlist_mtx = XCALLOC(MTYPE_PTHREAD, sizeof(pthread_mutex_t)); + peerlist_cond = XCALLOC(MTYPE_PTHREAD, sizeof(pthread_cond_t)); - // initialize synchronization primitives - pthread_mutex_lock(&peerlist_mtx); + // 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_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}; + + pthread_mutex_lock(peerlist_mtx); - // register cleanup handlers - pthread_cleanup_push(&cleanup_handler, NULL); + // register cleanup handler + pthread_cleanup_push(&peer_keepalives_finish, 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, - &peerlist_mtx); + pthread_cond_wait(peerlist_cond, 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); } diff --git a/bgpd/bgp_keepalives.h b/bgpd/bgp_keepalives.h index 281c32f173..2bd3964dbe 100644 --- a/bgpd/bgp_keepalives.h +++ b/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 diff --git a/bgpd/bgp_main.c b/bgpd/bgp_main.c index 066733989a..7dd4253b2e 100644 --- a/bgpd/bgp_main.c +++ b/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. */ diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 7d9d2426d0..0ff74a5ade 100644 --- a/bgpd/bgp_packet.c +++ b/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_cond_t write_cond = PTHREAD_COND_INITIALIZER; +static pthread_mutex_t *plist_mtx; +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); } diff --git a/bgpd/bgp_packet.h b/bgpd/bgp_packet.h index c18e4a2ebd..bedfa629fc 100644 --- a/bgpd/bgp_packet.h +++ b/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); diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 962324712f..f2cb8f81f7 100644 --- a/bgpd/bgpd.c +++ b/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); diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index 70a1d386c6..0767db58f3 100644 --- a/bgpd/bgpd.h +++ b/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 *); diff --git a/tests/bgpd/test_aspath.c b/tests/bgpd/test_aspath.c index 46462d79c4..5a508760b8 100644 --- a/tests/bgpd/test_aspath.c +++ b/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); diff --git a/tests/bgpd/test_capability.c b/tests/bgpd/test_capability.c index e8700a8b4a..05c409647f 100644 --- a/tests/bgpd/test_capability.c +++ b/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)); -- 2.39.5