From 3f3386cfe0bc425e0d6d4be6d3d88359acc3e95c Mon Sep 17 00:00:00 2001 From: Jason Dellaluce Date: Mon, 10 Oct 2022 10:31:39 +0000 Subject: [PATCH] fix(userspace/falco): make signal handlers safe with multi-threading Signed-off-by: Jason Dellaluce --- .../app_actions/create_signal_handlers.cpp | 48 ++++++++-------- .../falco/app_actions/process_events.cpp | 13 ++++- userspace/falco/application.cpp | 56 +++++++++++++++---- userspace/falco/application.h | 37 ++++++++---- 4 files changed, 108 insertions(+), 46 deletions(-) diff --git a/userspace/falco/app_actions/create_signal_handlers.cpp b/userspace/falco/app_actions/create_signal_handlers.cpp index 82ca9900..1d318b87 100644 --- a/userspace/falco/app_actions/create_signal_handlers.cpp +++ b/userspace/falco/app_actions/create_signal_handlers.cpp @@ -30,26 +30,24 @@ using namespace falco::app; // provided application, and in unregister_signal_handlers it will be // rebound back to the dummy application. -static application dummy; -static std::reference_wrapper s_app = dummy; static int inot_fd; -static void signal_callback(int signal) +static void terminate_signal_handler(int signal) { - falco_logger::log(LOG_INFO, "SIGINT received, exiting...\n"); - s_app.get().terminate(); + ASSERT(falco::app::g_terminate.is_lock_free()); + falco::app::g_terminate.store(APP_SIGNAL_SET, std::memory_order_seq_cst); } -static void reopen_outputs(int signal) +static void reopen_outputs_signal_handler(int signal) { - falco_logger::log(LOG_INFO, "SIGUSR1 received, reopening outputs...\n"); - s_app.get().reopen_outputs(); + ASSERT(falco::app::g_reopen_outputs.is_lock_free()); + falco::app::g_reopen_outputs.store(APP_SIGNAL_SET, std::memory_order_seq_cst); } -static void restart_falco(int signal) +static void restart_signal_handler(int signal) { - falco_logger::log(LOG_INFO, "SIGHUP received, restarting...\n"); - s_app.get().restart(); + ASSERT(falco::app::g_restart.is_lock_free()); + falco::app::g_restart.store(APP_SIGNAL_SET, std::memory_order_seq_cst); } bool application::create_handler(int sig, void (*func)(int), run_result &ret) @@ -74,21 +72,29 @@ bool application::create_handler(int sig, void (*func)(int), run_result &ret) application::run_result application::create_signal_handlers() { - run_result ret; - s_app = *this; - if(! create_handler(SIGINT, ::signal_callback, ret) || - ! create_handler(SIGTERM, ::signal_callback, ret) || - ! create_handler(SIGUSR1, ::reopen_outputs, ret) || - ! create_handler(SIGHUP, ::restart_falco, ret)) + falco::app::g_terminate.store(APP_SIGNAL_NOT_SET, std::memory_order_seq_cst); + falco::app::g_restart.store(APP_SIGNAL_NOT_SET, std::memory_order_seq_cst); + falco::app::g_reopen_outputs.store(APP_SIGNAL_NOT_SET, std::memory_order_seq_cst); + + if (!g_terminate.is_lock_free() + || !g_restart.is_lock_free() + || !g_reopen_outputs.is_lock_free()) { - s_app = dummy; + falco_logger::log(LOG_WARNING, "Bundled atomics implementation is not lock-free, signal handlers may be unstable\n"); } + + // we use the if just to make sure we return at the first failed statement + run_result ret; + if(! create_handler(SIGINT, ::terminate_signal_handler, ret) || + ! create_handler(SIGTERM, ::terminate_signal_handler, ret) || + ! create_handler(SIGUSR1, ::reopen_outputs_signal_handler, ret) || + ! create_handler(SIGHUP, ::restart_signal_handler, ret)); return ret; } application::run_result application::attach_inotify_signals() { - if (m_state->config->m_watch_config_files) + if (m_state->config->m_watch_config_files) { inot_fd = inotify_init(); if (inot_fd == -1) @@ -99,7 +105,7 @@ application::run_result application::attach_inotify_signals() struct sigaction sa; sigemptyset(&sa.sa_mask); sa.sa_flags = SA_RESTART; - sa.sa_handler = restart_falco; + sa.sa_handler = restart_signal_handler; if (sigaction(SIGIO, &sa, NULL) == -1) { return run_result::fatal("Failed to link SIGIO to inotify handler"); @@ -169,7 +175,5 @@ bool application::unregister_signal_handlers(std::string &errstr) errstr = ret.errstr; return false; } - - s_app = dummy; return true; } diff --git a/userspace/falco/app_actions/process_events.cpp b/userspace/falco/app_actions/process_events.cpp index 74563919..214f8bfc 100644 --- a/userspace/falco/app_actions/process_events.cpp +++ b/userspace/falco/app_actions/process_events.cpp @@ -97,11 +97,20 @@ application::run_result application::do_inspect( { rc = inspector->next(&ev); - if(m_state->terminate.load(std::memory_order_seq_cst) - || m_state->restart.load(std::memory_order_seq_cst)) + if(should_terminate()) { + terminate(); break; } + else if(should_restart()) + { + restart(); + break; + } + else if (should_reopen_outputs()) + { + reopen_outputs(); + } else if(rc == SCAP_TIMEOUT) { if(unlikely(ev == nullptr)) diff --git a/userspace/falco/application.cpp b/userspace/falco/application.cpp index 43933993..e44ee5ca 100644 --- a/userspace/falco/application.cpp +++ b/userspace/falco/application.cpp @@ -26,9 +26,41 @@ limitations under the License. using namespace std::placeholders; +static inline bool should_take_action_to_signal(std::atomic& v) +{ + // we expected the signal to be received, and we try to set action-taken flag + int value = APP_SIGNAL_SET; + while (!v.compare_exchange_weak( + value, + APP_SIGNAL_ACTION_TAKEN, + std::memory_order_seq_cst, + std::memory_order_seq_cst)) + { + // application already took action, there's no need to do it twice + if (value == APP_SIGNAL_ACTION_TAKEN) + { + return false; + } + + // signal did was not really received, so we "fake" receiving it + if (value == APP_SIGNAL_NOT_SET) + { + v.store(APP_SIGNAL_SET, std::memory_order_seq_cst); + } + + // reset "expected" CAS variable and keep looping until we succeed + value = APP_SIGNAL_SET; + } + return true; +} + namespace falco { namespace app { +std::atomic g_terminate(APP_SIGNAL_NOT_SET); +std::atomic g_restart(APP_SIGNAL_NOT_SET); +std::atomic g_reopen_outputs(APP_SIGNAL_NOT_SET); + application::run_result::run_result() : success(true), errstr(""), proceed(true) { @@ -39,9 +71,7 @@ application::run_result::~run_result() } application::state::state() - : restart(false), - terminate(false), - loaded_sources(), + : loaded_sources(), enabled_sources(), source_infos(), plugin_configs(), @@ -70,27 +100,29 @@ application::~application() void application::terminate() { - if(m_state != nullptr) + if (should_take_action_to_signal(falco::app::g_terminate)) { - m_state->terminate.store(true, std::memory_order_seq_cst); + falco_logger::log(LOG_INFO, "SIGINT received, exiting...\n"); } } void application::reopen_outputs() { - if(m_state != nullptr && m_state->outputs != nullptr) + if (should_take_action_to_signal(falco::app::g_reopen_outputs)) { - // note: it is ok to do this inside the signal handler because - // in the current falco_outputs implementation this is non-blocking - m_state->outputs->reopen_outputs(); + falco_logger::log(LOG_INFO, "SIGUSR1 received, reopening outputs...\n"); + if(m_state != nullptr && m_state->outputs != nullptr) + { + m_state->outputs->reopen_outputs(); + } } } void application::restart() { - if(m_state != nullptr) + if (should_take_action_to_signal(falco::app::g_restart)) { - m_state->restart.store(true, std::memory_order_seq_cst); + falco_logger::log(LOG_INFO, "SIGHUP received, restarting...\n"); } } @@ -196,7 +228,7 @@ bool application::run(std::string &errstr, bool &restart) errstr = res.errstr; } - restart = m_state->restart; + restart = should_restart(); return res.success; } diff --git a/userspace/falco/application.h b/userspace/falco/application.h index 13bc3aaa..9b123fa4 100644 --- a/userspace/falco/application.h +++ b/userspace/falco/application.h @@ -30,9 +30,19 @@ limitations under the License. #include #include +#define APP_SIGNAL_NOT_SET 0 // The signal flag is not set +#define APP_SIGNAL_SET 1 // The signal flag has been set +#define APP_SIGNAL_ACTION_TAKEN 2 // The signal flag has been set and the application took action + namespace falco { namespace app { +// these are used to control the lifecycle of the application +// through signal handlers or internal calls +extern std::atomic g_terminate; +extern std::atomic g_restart; +extern std::atomic g_reopen_outputs; + class application { public: application(); @@ -42,13 +52,6 @@ public: application(const application&) = delete; application& operator = (const application&) = delete; - // These are only used in signal handlers. Other than there, - // the control flow of the application should not be changed - // from the outside. - void terminate(); - void reopen_outputs(); - void restart(); - bool init(int argc, char **argv, std::string &errstr); // Returns whether the application completed with errors or @@ -86,9 +89,6 @@ private: state(); virtual ~state(); - std::atomic restart; - std::atomic terminate; - std::shared_ptr config; std::shared_ptr outputs; std::shared_ptr engine; @@ -317,6 +317,23 @@ private: return !m_options.gvisor_config.empty(); } + // used in signal handlers to control the flow of the application + void terminate(); + void restart(); + void reopen_outputs(); + inline bool should_terminate() + { + return g_terminate.load(std::memory_order_seq_cst) != APP_SIGNAL_NOT_SET; + } + inline bool should_restart() + { + return g_restart.load(std::memory_order_seq_cst) != APP_SIGNAL_NOT_SET; + } + inline bool should_reopen_outputs() + { + return g_reopen_outputs.load(std::memory_order_seq_cst) != APP_SIGNAL_NOT_SET; + } + std::unique_ptr m_state; cmdline_options m_options; bool m_initialized;