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

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 = \

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

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 \

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");
 	}
 }
 

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;
+}
+
+

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");
 	}
 }
 

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");
 	}
 }