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 277e4e69b0032060e7cd652a902d03f12d50f7d4..b3aa67714a13375101f210d7f873b244c59a4457 100644 (file)
--- 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 (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/liblttng-ust-comm/Makefile.am b/liblttng-ust-comm/Makefile.am
index b904306587301285a2da365d00652b3e42c22f7b..da77d9ceb8a0ea19d805a1f6fe9baef6bb20426a 100644 (file)
--- 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 9659f349e54ac85a3afad50b2cfc2055abcfacfc..9909c0606a45044ab33f37ed09603d87d60dcf3c 100644 (file)
--- a/liblttng-ust-comm/lttng-ust-fd-tracker.c
+++ b/liblttng-ust-comm/lttng-ust-fd-tracker.c
@@ -42,6 +42,7 @@
 #include <helper.h>
 #include <lttng/ust-error.h>
 #include <usterr-signal-safe.h>
+#include <lttng/ust-cancelstate.h>
 
 #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 (file)
index 0000000..298ffcb
--- /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 <mathieu.desnoyers@efficios.com>
+ */
+
+#include <pthread.h>
+#include <errno.h>
+#include <error.h>
+#include <urcu/tls-compat.h>
+#include <lttng/ust-cancelstate.h>
+#include <usterr-signal-safe.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/liblttng-ust/lttng-context-perf-counters.c b/liblttng-ust/lttng-context-perf-counters.c
index a6ff55b678d472d29fb157f7b0527918432121e2..36269c245f962376c661e5464c7f12239d9cfb92 100644 (file)
--- a/liblttng-ust/lttng-context-perf-counters.c
+++ b/liblttng-ust/lttng-context-perf-counters.c
@@ -32,6 +32,7 @@
 #include <lttng/ust-events.h>
 #include <lttng/ust-tracer.h>
 #include <lttng/ringbuffer-config.h>
+#include <lttng/ust-cancelstate.h>
 #include <urcu/system.h>
 #include <urcu/arch.h>
 #include <urcu/rculist.h>
@@ -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 6f3a58aaf57674226879f646299f71ab83a3261a..f3222699d7814395ac60a78fa22c445df2c91c5b 100644 (file)
--- a/liblttng-ust/lttng-ust-comm.c
+++ b/liblttng-ust/lttng-ust-comm.c
@@ -46,6 +46,7 @@
 #include <lttng/ust.h>
 #include <lttng/ust-error.h>
 #include <lttng/ust-ctl.h>
+#include <lttng/ust-cancelstate.h>
 #include <urcu/tls-compat.h>
 #include <ust-comm.h>
 #include <ust-fd.h>
@@ -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");
        }
 }