From 59e57035d48effd595bd68806bd771c85ec7d406 Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Thu, 9 Sep 2021 12:49:26 -0400 Subject: [PATCH] Fix: nestable pthread cancelstate The pthread cancelstate disable performed to ensure threads are not cancelled while holding mutexes which are used in library destructors does not currently support that those mutexes may be nested. It generates error messages when using the fork and fd helpers when running with LTTNG_UST_DEBUG=1. The effect of this is that the pthread cancelstate can be re-enabled too soon when the first unlock is performed (in a nested lock scenario), thus allowing the thread to be cancelled while still holding a lock, and causing a deadlock on application exit. Signed-off-by: Mathieu Desnoyers Change-Id: Ife8b1fee04c7d7c480e59bdfc158abdee771994c --- include/Makefile.am | 3 +- include/lttng/ust-cancelstate.h | 13 ++++ src/lib/lttng-ust-common/Makefile.am | 3 +- src/lib/lttng-ust-common/fd-tracker.c | 27 +++----- src/lib/lttng-ust-common/ust-cancelstate.c | 61 +++++++++++++++++++ .../lttng-ust/lttng-context-perf-counters.c | 27 +++----- src/lib/lttng-ust/lttng-ust-comm.c | 31 +++------- 7 files changed, 102 insertions(+), 63 deletions(-) create mode 100644 include/lttng/ust-cancelstate.h create mode 100644 src/lib/lttng-ust-common/ust-cancelstate.c diff --git a/include/Makefile.am b/include/Makefile.am index b1f5dce9..185ae01f 100644 --- a/include/Makefile.am +++ b/include/Makefile.am @@ -38,7 +38,8 @@ nobase_include_HEADERS = \ lttng/urcu/urcu-ust.h \ lttng/urcu/static/pointer.h \ lttng/urcu/static/urcu-ust.h \ - lttng/ust-sigbus.h + lttng/ust-sigbus.h \ + lttng/ust-cancelstate.h # Auto-generated by configure. nobase_nodist_include_HEADERS = \ diff --git a/include/lttng/ust-cancelstate.h b/include/lttng/ust-cancelstate.h new file mode 100644 index 00000000..efca9ac6 --- /dev/null +++ b/include/lttng/ust-cancelstate.h @@ -0,0 +1,13 @@ +/* + * SPDX-License-Identifier: LGPL-2.1-only + * + * Copyright (C) 2021 Mathieu Desnoyers + */ + +#ifndef _LTTNG_UST_UST_CANCELSTATE_H +#define _LTTNG_UST_UST_CANCELSTATE_H + +int lttng_ust_cancelstate_disable_push(void); +int lttng_ust_cancelstate_disable_pop(void); + +#endif diff --git a/src/lib/lttng-ust-common/Makefile.am b/src/lib/lttng-ust-common/Makefile.am index caeea2be..1ccc290a 100644 --- a/src/lib/lttng-ust-common/Makefile.am +++ b/src/lib/lttng-ust-common/Makefile.am @@ -11,7 +11,8 @@ liblttng_ust_common_la_SOURCES = \ getcpu.h \ ust-common.c \ lttng-ust-urcu.c \ - lttng-ust-urcu-pointer.c + lttng-ust-urcu-pointer.c \ + ust-cancelstate.c liblttng_ust_common_la_LIBADD = \ $(top_builddir)/src/common/libcommon.la \ diff --git a/src/lib/lttng-ust-common/fd-tracker.c b/src/lib/lttng-ust-common/fd-tracker.c index 7a17e2a0..70539cab 100644 --- a/src/lib/lttng-ust-common/fd-tracker.c +++ b/src/lib/lttng-ust-common/fd-tracker.c @@ -28,6 +28,7 @@ #include "common/ust-fd.h" #include "common/macros.h" #include +#include #include "common/logging.h" #include "lib/lttng-ust-common/fd-tracker.h" @@ -56,12 +57,6 @@ */ static pthread_mutex_t ust_safe_guard_fd_mutex = PTHREAD_MUTEX_INITIALIZER; -/* - * Cancel state when grabbing the ust_safe_guard_fd_mutex. Saved when - * locking, restored on unlock. Protected by ust_safe_guard_fd_mutex. - */ -static int ust_safe_guard_saved_cancelstate; - /* * Track whether we are within lttng-ust or application, for close * system call override by LD_PRELOAD library. This also tracks whether @@ -126,11 +121,10 @@ void lttng_ust_fd_tracker_init(void) void lttng_ust_lock_fd_tracker(void) { sigset_t sig_all_blocked, orig_mask; - int ret, oldstate; + int ret; - ret = pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldstate); - if (ret) { - ERR("pthread_setcancelstate: %s", strerror(ret)); + if (lttng_ust_cancelstate_disable_push()) { + ERR("lttng_ust_cancelstate_disable_push"); } sigfillset(&sig_all_blocked); ret = pthread_sigmask(SIG_SETMASK, &sig_all_blocked, &orig_mask); @@ -144,7 +138,6 @@ void lttng_ust_lock_fd_tracker(void) */ cmm_barrier(); pthread_mutex_lock(&ust_safe_guard_fd_mutex); - ust_safe_guard_saved_cancelstate = oldstate; } ret = pthread_sigmask(SIG_SETMASK, &orig_mask, NULL); if (ret) { @@ -155,8 +148,7 @@ void lttng_ust_lock_fd_tracker(void) void lttng_ust_unlock_fd_tracker(void) { sigset_t sig_all_blocked, orig_mask; - int ret, newstate, oldstate; - bool restore_cancel = false; + int ret; sigfillset(&sig_all_blocked); ret = pthread_sigmask(SIG_SETMASK, &sig_all_blocked, &orig_mask); @@ -169,19 +161,14 @@ void lttng_ust_unlock_fd_tracker(void) */ cmm_barrier(); if (!--URCU_TLS(ust_fd_mutex_nest)) { - newstate = ust_safe_guard_saved_cancelstate; - restore_cancel = true; pthread_mutex_unlock(&ust_safe_guard_fd_mutex); } ret = pthread_sigmask(SIG_SETMASK, &orig_mask, NULL); if (ret) { ERR("pthread_sigmask: %s", strerror(ret)); } - if (restore_cancel) { - ret = pthread_setcancelstate(newstate, &oldstate); - if (ret) { - ERR("pthread_setcancelstate: %s", strerror(ret)); - } + if (lttng_ust_cancelstate_disable_pop()) { + ERR("lttng_ust_cancelstate_disable_pop"); } } diff --git a/src/lib/lttng-ust-common/ust-cancelstate.c b/src/lib/lttng-ust-common/ust-cancelstate.c new file mode 100644 index 00000000..2805758b --- /dev/null +++ b/src/lib/lttng-ust-common/ust-cancelstate.c @@ -0,0 +1,61 @@ +/* + * SPDX-License-Identifier: LGPL-2.1-only + * + * Copyright (C) 2021 Mathieu Desnoyers + */ + +#include +#include +#include +#include +#include + +#include "common/logging.h" + +struct ust_cancelstate { + int nesting; + int oldstate; /* oldstate for outermost nesting */ +}; + +static DEFINE_URCU_TLS(struct ust_cancelstate, thread_state); + +int lttng_ust_cancelstate_disable_push(void) +{ + struct ust_cancelstate *state = &URCU_TLS(thread_state); + int ret, oldstate; + + if (state->nesting++) + goto end; + ret = pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldstate); + if (ret) { + ERR("pthread_setcancelstate: %s", strerror(ret)); + return -1; + } + state->oldstate = oldstate; +end: + return 0; +} + +int lttng_ust_cancelstate_disable_pop(void) +{ + struct ust_cancelstate *state = &URCU_TLS(thread_state); + int ret, oldstate; + + if (!state->nesting) + return -1; + if (--state->nesting) + goto end; + ret = pthread_setcancelstate(state->oldstate, &oldstate); + if (ret) { + ERR("pthread_setcancelstate: %s", strerror(ret)); + return -1; + } + if (oldstate != PTHREAD_CANCEL_DISABLE) { + ERR("pthread_setcancelstate: unexpected oldstate"); + return -1; + } +end: + return 0; +} + + diff --git a/src/lib/lttng-ust/lttng-context-perf-counters.c b/src/lib/lttng-ust/lttng-context-perf-counters.c index 233c654c..2db11436 100644 --- a/src/lib/lttng-ust/lttng-context-perf-counters.c +++ b/src/lib/lttng-ust/lttng-context-perf-counters.c @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -77,12 +78,6 @@ static pthread_key_t perf_counter_key; */ static pthread_mutex_t ust_perf_mutex = PTHREAD_MUTEX_INITIALIZER; -/* - * Cancel state when grabbing the ust_perf_mutex. Saved when locking, - * restored on unlock. Protected by ust_perf_mutex. - */ -static int ust_perf_saved_cancelstate; - /* * Track whether we are tracing from a signal handler nested on an * application thread. @@ -100,11 +95,10 @@ void lttng_ust_perf_counter_alloc_tls(void) void lttng_perf_lock(void) { sigset_t sig_all_blocked, orig_mask; - int ret, oldstate; + int ret; - ret = pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldstate); - if (ret) { - ERR("pthread_setcancelstate: %s", strerror(ret)); + if (lttng_ust_cancelstate_disable_push()) { + ERR("lttng_ust_cancelstate_disable_push"); } sigfillset(&sig_all_blocked); ret = pthread_sigmask(SIG_SETMASK, &sig_all_blocked, &orig_mask); @@ -118,7 +112,6 @@ void lttng_perf_lock(void) */ cmm_barrier(); pthread_mutex_lock(&ust_perf_mutex); - ust_perf_saved_cancelstate = oldstate; } ret = pthread_sigmask(SIG_SETMASK, &orig_mask, NULL); if (ret) { @@ -129,8 +122,7 @@ void lttng_perf_lock(void) void lttng_perf_unlock(void) { sigset_t sig_all_blocked, orig_mask; - int ret, newstate, oldstate; - bool restore_cancel = false; + int ret; sigfillset(&sig_all_blocked); ret = pthread_sigmask(SIG_SETMASK, &sig_all_blocked, &orig_mask); @@ -143,19 +135,14 @@ void lttng_perf_unlock(void) */ cmm_barrier(); if (!--URCU_TLS(ust_perf_mutex_nest)) { - newstate = ust_perf_saved_cancelstate; - restore_cancel = true; pthread_mutex_unlock(&ust_perf_mutex); } ret = pthread_sigmask(SIG_SETMASK, &orig_mask, NULL); if (ret) { ERR("pthread_sigmask: %s", strerror(ret)); } - if (restore_cancel) { - ret = pthread_setcancelstate(newstate, &oldstate); - if (ret) { - ERR("pthread_setcancelstate: %s", strerror(ret)); - } + if (lttng_ust_cancelstate_disable_pop()) { + ERR("lttng_ust_cancelstate_disable_pop"); } } diff --git a/src/lib/lttng-ust/lttng-ust-comm.c b/src/lib/lttng-ust/lttng-ust-comm.c index c6021cce..8ded320e 100644 --- a/src/lib/lttng-ust/lttng-ust-comm.c +++ b/src/lib/lttng-ust/lttng-ust-comm.c @@ -38,6 +38,7 @@ #include #include #include +#include #include #include "lib/lttng-ust/futex.h" #include "common/ustcomm.h" @@ -125,14 +126,10 @@ int lttng_ust_loaded __attribute__((weak)); int ust_lock(void) { sigset_t sig_all_blocked, orig_mask; - int ret, oldstate; + int ret; - ret = pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldstate); - if (ret) { - ERR("pthread_setcancelstate: %s", strerror(ret)); - } - if (oldstate != PTHREAD_CANCEL_ENABLE) { - ERR("pthread_setcancelstate: unexpected oldstate"); + if (lttng_ust_cancelstate_disable_push()) { + ERR("lttng_ust_cancelstate_disable_push"); } sigfillset(&sig_all_blocked); ret = pthread_sigmask(SIG_SETMASK, &sig_all_blocked, &orig_mask); @@ -161,14 +158,10 @@ int ust_lock(void) void ust_lock_nocheck(void) { sigset_t sig_all_blocked, orig_mask; - int ret, oldstate; + int ret; - ret = pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldstate); - if (ret) { - ERR("pthread_setcancelstate: %s", strerror(ret)); - } - if (oldstate != PTHREAD_CANCEL_ENABLE) { - ERR("pthread_setcancelstate: unexpected oldstate"); + if (lttng_ust_cancelstate_disable_push()) { + ERR("lttng_ust_cancelstate_disable_push"); } sigfillset(&sig_all_blocked); ret = pthread_sigmask(SIG_SETMASK, &sig_all_blocked, &orig_mask); @@ -189,7 +182,7 @@ void ust_lock_nocheck(void) void ust_unlock(void) { sigset_t sig_all_blocked, orig_mask; - int ret, oldstate; + int ret; sigfillset(&sig_all_blocked); ret = pthread_sigmask(SIG_SETMASK, &sig_all_blocked, &orig_mask); @@ -202,12 +195,8 @@ void ust_unlock(void) if (ret) { ERR("pthread_sigmask: %s", strerror(ret)); } - ret = pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, &oldstate); - if (ret) { - ERR("pthread_setcancelstate: %s", strerror(ret)); - } - if (oldstate != PTHREAD_CANCEL_DISABLE) { - ERR("pthread_setcancelstate: unexpected oldstate"); + if (lttng_ust_cancelstate_disable_pop()) { + ERR("lttng_ust_cancelstate_disable_pop"); } } -- 2.39.5