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 <mathieu.desnoyers@efficios.com>
Change-Id: Ife8b1fee04c7d7c480e59bdfc158abdee771994c

diff --git a/include/Makefile.am b/include/Makefile.am
index b1f5dce9dc27a4d3d6b57d0c557379f4bdac44df..185ae01f23769cc559d74050c365f444f8d6c4e6 100644 (file)
--- 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 (file)
index 0000000..efca9ac
--- /dev/null
+++ b/include/lttng/ust-cancelstate.h
@@ -0,0 +1,13 @@
+/*
+ * SPDX-License-Identifier: LGPL-2.1-only
+ *
+ * Copyright (C) 2021 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
+ */
+
+#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 caeea2be5de74e54d7d67ddc0a4e45e9a429cbe1..1ccc290a4302c4c0c29677c0fbf3beea512fd3a1 100644 (file)
--- 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 b2d881a365a954e8ed3bc0c166f87a7e8c9bc1b4..8638ea021d5eada2d857b247d94aa2e44dbfb256 100644 (file)
--- 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 <lttng/ust-error.h>
+#include <lttng/ust-cancelstate.h>
 #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 (file)
index 0000000..2805758
--- /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 <mathieu.desnoyers@efficios.com>
+ */
+
+#include <pthread.h>
+#include <errno.h>
+#include <error.h>
+#include <urcu/tls-compat.h>
+#include <lttng/ust-cancelstate.h>
+
+#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 233c654ca75a3237973043a46a882670f5769a0c..2db11436a9ad8fa320f7092d9420e21972dc2ad6 100644 (file)
--- 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 <lttng/ust-events.h>
 #include <lttng/ust-tracer.h>
 #include <lttng/ust-ringbuffer-context.h>
+#include <lttng/ust-cancelstate.h>
 #include <urcu/system.h>
 #include <urcu/arch.h>
 #include <urcu/rculist.h>
@@ -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 25d79d9f4dce4fc7f3f4518e796e270fa8fcf78a..d67550797c5443aef3d6342db83c612450458cdb 100644 (file)
--- a/src/lib/lttng-ust/lttng-ust-comm.c
+++ b/src/lib/lttng-ust/lttng-ust-comm.c
@@ -38,6 +38,7 @@
 #include <lttng/ust-thread.h>
 #include <lttng/ust-tracer.h>
 #include <lttng/ust-common.h>
+#include <lttng/ust-cancelstate.h>
 #include <urcu/tls-compat.h>
 #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");
        }
 }