From: Mathieu Desnoyers Date: Thu, 9 Sep 2021 16:49:26 +0000 (-0400) Subject: Fix: nestable pthread cancelstate X-Git-Tag: v2.12.3~3 X-Git-Url: https://git.lttng.org./?a=commitdiff_plain;h=595c15773def8e249145c4dead560ba7329810c7;p=lttng-ust.git 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 --- diff --git a/include/Makefile.am b/include/Makefile.am index 277e4e69..b3aa6771 100644 --- a/include/Makefile.am +++ b/include/Makefile.am @@ -36,6 +36,7 @@ noinst_HEADERS = \ ust-fd.h \ lttng/ust-tid.h \ lttng/bitfield.h \ + lttng/ust-cancelstate.h \ lttng/ust-dlfcn.h \ lttng/ust-dynamic-type.h \ lttng/ust-context-provider.h \ 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/liblttng-ust-comm/Makefile.am b/liblttng-ust-comm/Makefile.am index b9043065..da77d9ce 100644 --- a/liblttng-ust-comm/Makefile.am +++ b/liblttng-ust-comm/Makefile.am @@ -2,4 +2,4 @@ AM_CPPFLAGS = -I$(top_builddir)/include -I$(top_srcdir)/include noinst_LTLIBRARIES = liblttng-ust-comm.la -liblttng_ust_comm_la_SOURCES = lttng-ust-comm.c lttng-ust-fd-tracker.c +liblttng_ust_comm_la_SOURCES = lttng-ust-comm.c lttng-ust-fd-tracker.c ust-cancelstate.c diff --git a/liblttng-ust-comm/lttng-ust-fd-tracker.c b/liblttng-ust-comm/lttng-ust-fd-tracker.c index 9659f349..9909c060 100644 --- a/liblttng-ust-comm/lttng-ust-fd-tracker.c +++ b/liblttng-ust-comm/lttng-ust-fd-tracker.c @@ -42,6 +42,7 @@ #include #include #include +#include #include "../liblttng-ust/compat.h" @@ -69,12 +70,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 @@ -139,11 +134,10 @@ void lttng_ust_init_fd_tracker(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); @@ -157,7 +151,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) { @@ -168,8 +161,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); @@ -182,19 +174,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/liblttng-ust-comm/ust-cancelstate.c b/liblttng-ust-comm/ust-cancelstate.c new file mode 100644 index 00000000..298ffcbe --- /dev/null +++ b/liblttng-ust-comm/ust-cancelstate.c @@ -0,0 +1,60 @@ +/* + * SPDX-License-Identifier: LGPL-2.1-only + * + * Copyright (C) 2021 Mathieu Desnoyers + */ + +#include +#include +#include +#include +#include +#include + +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/liblttng-ust/lttng-context-perf-counters.c b/liblttng-ust/lttng-context-perf-counters.c index a6ff55b6..36269c24 100644 --- a/liblttng-ust/lttng-context-perf-counters.c +++ b/liblttng-ust/lttng-context-perf-counters.c @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -82,12 +83,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. @@ -105,11 +100,10 @@ void lttng_ust_fixup_perf_counter_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); @@ -123,7 +117,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) { @@ -134,8 +127,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); @@ -148,19 +140,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/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c index 6f3a58aa..f3222699 100644 --- a/liblttng-ust/lttng-ust-comm.c +++ b/liblttng-ust/lttng-ust-comm.c @@ -46,6 +46,7 @@ #include #include #include +#include #include #include #include @@ -130,14 +131,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); @@ -166,14 +163,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); @@ -194,7 +187,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); @@ -207,12 +200,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"); } }