From 2bafda27a602f74a592551bf695438218efd62f4 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 22 Apr 2021 12:06:51 +0200 Subject: [PATCH 1/5] lib: rename very_late_init hook to config_post very_late_init doesn't really say what this does, config_post is much more descriptive. (A config_pre is coming in a jiffy.) Signed-off-by: David Lamparter --- lib/libfrr.c | 4 ++-- lib/libfrr.h | 2 +- lib/northbound_sysrepo.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/libfrr.c b/lib/libfrr.c index c8f2540db5..ee02e019e3 100644 --- a/lib/libfrr.c +++ b/lib/libfrr.c @@ -46,7 +46,7 @@ #include "frrscript.h" DEFINE_HOOK(frr_late_init, (struct thread_master * tm), (tm)); -DEFINE_HOOK(frr_very_late_init, (struct thread_master * tm), (tm)); +DEFINE_HOOK(frr_config_post, (struct thread_master * tm), (tm)); DEFINE_KOOH(frr_early_fini, (), ()); DEFINE_KOOH(frr_fini, (), ()); @@ -964,7 +964,7 @@ static int frr_config_read_in(struct thread *t) __func__, nb_err_name(ret), errmsg); } - hook_call(frr_very_late_init, master); + hook_call(frr_config_post, master); return 0; } diff --git a/lib/libfrr.h b/lib/libfrr.h index db0f364986..114e631226 100644 --- a/lib/libfrr.h +++ b/lib/libfrr.h @@ -142,7 +142,7 @@ extern uint32_t frr_get_fd_limit(void); extern bool frr_is_startup_fd(int fd); DECLARE_HOOK(frr_late_init, (struct thread_master * tm), (tm)); -DECLARE_HOOK(frr_very_late_init, (struct thread_master * tm), (tm)); +DECLARE_HOOK(frr_config_post, (struct thread_master * tm), (tm)); extern void frr_config_fork(void); extern void frr_run(struct thread_master *master); diff --git a/lib/northbound_sysrepo.c b/lib/northbound_sysrepo.c index fc1af092d0..63fd40f8d3 100644 --- a/lib/northbound_sysrepo.c +++ b/lib/northbound_sysrepo.c @@ -736,7 +736,7 @@ static int frr_sr_finish(void) return 0; } -static int frr_sr_module_very_late_init(struct thread_master *tm) +static int frr_sr_module_config_loaded(struct thread_master *tm) { master = tm; @@ -761,7 +761,7 @@ static int frr_sr_module_late_init(struct thread_master *tm) static int frr_sr_module_init(void) { hook_register(frr_late_init, frr_sr_module_late_init); - hook_register(frr_very_late_init, frr_sr_module_very_late_init); + hook_register(frr_config_post, frr_sr_module_config_loaded); return 0; } From bf645e31f696a42f60fbd5361beefd4ee707b25e Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 22 Apr 2021 13:18:19 +0200 Subject: [PATCH 2/5] lib: add frr_config_pre hook ... for any initialization that needs to run after forking, but that would be racy if it were just scheduled on the thread_master (since the config load is also just a thread callback, ordering would be undefined for another scheduled thread callback.) Signed-off-by: David Lamparter --- lib/libfrr.c | 3 +++ lib/libfrr.h | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/lib/libfrr.c b/lib/libfrr.c index ee02e019e3..38f994bdfc 100644 --- a/lib/libfrr.c +++ b/lib/libfrr.c @@ -46,6 +46,7 @@ #include "frrscript.h" DEFINE_HOOK(frr_late_init, (struct thread_master * tm), (tm)); +DEFINE_HOOK(frr_config_pre, (struct thread_master * tm), (tm)); DEFINE_HOOK(frr_config_post, (struct thread_master * tm), (tm)); DEFINE_KOOH(frr_early_fini, (), ()); DEFINE_KOOH(frr_fini, (), ()); @@ -931,6 +932,8 @@ static void frr_daemonize(void) */ static int frr_config_read_in(struct thread *t) { + hook_call(frr_config_pre, master); + if (!vty_read_config(vty_shared_candidate_config, di->config_file, config_default) && di->backup_config_file) { diff --git a/lib/libfrr.h b/lib/libfrr.h index 114e631226..47ded8f313 100644 --- a/lib/libfrr.h +++ b/lib/libfrr.h @@ -141,8 +141,12 @@ extern enum frr_cli_mode frr_get_cli_mode(void); extern uint32_t frr_get_fd_limit(void); extern bool frr_is_startup_fd(int fd); +/* call order of these hooks is as ordered here */ DECLARE_HOOK(frr_late_init, (struct thread_master * tm), (tm)); +/* fork() happens between late_init and config_pre */ +DECLARE_HOOK(frr_config_pre, (struct thread_master * tm), (tm)); DECLARE_HOOK(frr_config_post, (struct thread_master * tm), (tm)); + extern void frr_config_fork(void); extern void frr_run(struct thread_master *master); From 159246be247fd4332780fcafe5909874ae2c705a Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 22 Apr 2021 12:17:42 +0200 Subject: [PATCH 3/5] pathd: don't init PCEP before fork() Turns out the PCEP stuff does not work particularly well if its threads are ... missing. Who would've thought? Reported-by: Erik Kooistra Signed-off-by: David Lamparter --- pathd/path_pcep.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/pathd/path_pcep.c b/pathd/path_pcep.c index 1c650737b2..ad24c2eb02 100644 --- a/pathd/path_pcep.c +++ b/pathd/path_pcep.c @@ -264,7 +264,11 @@ int pathd_candidate_removed_handler(struct srte_candidate *candidate) /* ------------ Module Functions ------------ */ -int pcep_module_late_init(struct thread_master *tm) +/* this creates threads, therefore must run after fork(). but it must also + * run before config load, so the CLI commands don't try to touch things that + * aren't set up yet... + */ +static int pcep_module_config_pre(struct thread_master *tm) { assert(pcep_g->fpt == NULL); assert(pcep_g->master == NULL); @@ -280,10 +284,16 @@ int pcep_module_late_init(struct thread_master *tm) pcep_g->master = tm; pcep_g->fpt = fpt; + return 0; +} + +static int pcep_module_late_init(struct thread_master *tm) +{ hook_register(pathd_candidate_created, pathd_candidate_created_handler); hook_register(pathd_candidate_updated, pathd_candidate_updated_handler); hook_register(pathd_candidate_removed, pathd_candidate_removed_handler); + hook_register(frr_config_pre, pcep_module_config_pre); hook_register(frr_fini, pcep_module_finish); pcep_cli_init(); From 38554d3ae6c4ea553fd4e9ab2185c0d79f1144f7 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 22 Apr 2021 12:10:27 +0200 Subject: [PATCH 4/5] lib: hard-fail creating threads before fork() Creating any threads before we fork() into the background (if `-d` is given) is an extremely dangerous footgun; the threads are created in the parent and terminated when that exits. This is extra dangerous because while testing, you'd often run the daemon in foreground without `-d`, and everything works as expected. Signed-off-by: David Lamparter --- lib/frr_pthread.c | 3 +++ lib/libfrr.c | 5 +++++ lib/libfrr.h | 2 ++ 3 files changed, 10 insertions(+) diff --git a/lib/frr_pthread.c b/lib/frr_pthread.c index 03359f4d18..898fe98aad 100644 --- a/lib/frr_pthread.c +++ b/lib/frr_pthread.c @@ -28,6 +28,7 @@ #include "memory.h" #include "linklist.h" #include "zlog.h" +#include "libfrr.h" #include "libfrr_trace.h" DEFINE_MTYPE_STATIC(LIB, FRR_PTHREAD, "FRR POSIX Thread"); @@ -162,6 +163,8 @@ int frr_pthread_run(struct frr_pthread *fpt, const pthread_attr_t *attr) int ret; sigset_t oldsigs, blocksigs; + assert(frr_is_after_fork || !"trying to start thread before fork()"); + /* Ensure we never handle signals on a background thread by blocking * everything here (new thread inherits signal mask) */ diff --git a/lib/libfrr.c b/lib/libfrr.c index 38f994bdfc..970e82c064 100644 --- a/lib/libfrr.c +++ b/lib/libfrr.c @@ -70,6 +70,8 @@ static char dbfile_default[512]; #endif static char vtypath_default[512]; +/* cleared in frr_preinit(), then re-set after daemonizing */ +bool frr_is_after_fork = true; bool debug_memstats_at_exit = false; static bool nodetach_term, nodetach_daemon; static uint64_t startup_fds; @@ -308,6 +310,7 @@ void frr_init_vtydir(void) void frr_preinit(struct frr_daemon_info *daemon, int argc, char **argv) { di = daemon; + frr_is_after_fork = false; /* basename(), opencoded. */ char *p = strrchr(argv[0], '/'); @@ -990,6 +993,8 @@ void frr_config_fork(void) if (di->daemon_mode || di->terminal) frr_daemonize(); + frr_is_after_fork = true; + if (!di->pid_file) di->pid_file = pidfile_default; pid_output(di->pid_file); diff --git a/lib/libfrr.h b/lib/libfrr.h index 47ded8f313..3dc5d7af81 100644 --- a/lib/libfrr.h +++ b/lib/libfrr.h @@ -172,6 +172,8 @@ extern const char frr_scriptdir[]; extern char frr_protoname[]; extern char frr_protonameinst[]; +/* always set in the spot where we *would* fork even if we don't do so */ +extern bool frr_is_after_fork; extern bool debug_memstats_at_exit; From adf8924df66f9249e1294eee365cdebf17c897d4 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Fri, 23 Apr 2021 15:17:07 +0200 Subject: [PATCH 5/5] ldpd: set `frr_is_after_fork` in lde/ldpe These subprocesses don't use frr_config_fork(), so frr_is_after_fork is never set. While the frr_pthread stuff isn't currently used there, set the flag anyway to avoid future headaches. Signed-off-by: David Lamparter --- ldpd/lde.c | 2 ++ ldpd/ldpe.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/ldpd/lde.c b/ldpd/lde.c index 8fa74d1c3d..02dcec750b 100644 --- a/ldpd/lde.c +++ b/ldpd/lde.c @@ -134,6 +134,8 @@ lde(void) log_procname = log_procnames[PROC_LDE_ENGINE]; master = frr_init(); + /* no frr_config_fork() here, allow frr_pthread to create threads */ + frr_is_after_fork = true; /* setup signal handler */ signal_init(master, array_size(lde_signals), lde_signals); diff --git a/ldpd/ldpe.c b/ldpd/ldpe.c index d09eb2fa33..428d2ab7b4 100644 --- a/ldpd/ldpe.c +++ b/ldpd/ldpe.c @@ -111,6 +111,8 @@ ldpe(void) log_procname = log_procnames[ldpd_process]; master = frr_init(); + /* no frr_config_fork() here, allow frr_pthread to create threads */ + frr_is_after_fork = true; /* setup signal handler */ signal_init(master, array_size(ldpe_signals), ldpe_signals);