[lttng-dev] [RFC PATCH 2/2] Fix: order termination of ust_thread_manage_notify

[Jonathan Rajotte]

The following scenario lead to a deadlock:
 - An application is being registered sessiond side (thread_dispatch_ust_registration).
 - ust_thread_manage_notify is already terminated via quit pipe, hence
   not listening on the notify socket.
 - The application is trying to register an event (lttng_event_create->ustcomm_register_event) via the notify socket.
 - The notify socket is never closed.

The application hang on the communication via the notify socket since
no one is responding, sessiond is waiting for a reply on the
ustctl_register_done call.

Forcing an order of termination on ust_thread_manage_notify and
thread_dispatch_ust_registration guarantees that the socket will be
closed and allow for regular execution. This guarantee is provided by
the fact that no further registration can happen in the absence of the
registration dispatch thread and all current applications will be present
when the ust_thread_manage_notify perform its cleanup.

This can be reproduced using the sessiond_teardown_active_session
scenario provided by [1].

[1] https://github.com/PSRCode/lttng-stress

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
---
 src/bin/lttng-sessiond/lttng-sessiond.h |  7 +++++++
 src/bin/lttng-sessiond/main.c           | 24 +++++++++++++++++++++++-
 src/bin/lttng-sessiond/ust-thread.c     | 16 ++++++++++++----
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/src/bin/lttng-sessiond/lttng-sessiond.h b/src/bin/lttng-sessiond/lttng-sessiond.h
index 74552db6..a805704e 100644
--- a/src/bin/lttng-sessiond/lttng-sessiond.h
+++ b/src/bin/lttng-sessiond/lttng-sessiond.h
@@ -95,6 +95,13 @@ struct ust_reg_wait_node {
 extern int apps_cmd_notify_pipe[2];
 
 /*
+ * This pipe is used to inform the ust_thread_manage_notify thread that the
+ * thread_dispatch_ust_registration thread is terminated. Allowing the
+ * ust_thread_manage_notify to perform its termination.
+ */
+extern int thread_dispatch_ust_registration_teardown_complete_pipe[2];
+
+/*
  * Used to notify that a hash table needs to be destroyed by dedicated
  * thread. Required by design because we don't want to move destroy
  * paths outside of large RCU read-side lock paths, and destroy cannot
diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
index 03f695ec..5298b1a1 100644
--- a/src/bin/lttng-sessiond/main.c
+++ b/src/bin/lttng-sessiond/main.c
@@ -206,6 +206,13 @@ static int kernel_poll_pipe[2] = { -1, -1 };
 static int thread_quit_pipe[2] = { -1, -1 };
 
 /*
+ * This pipe is used to inform the ust_thread_manage_notify thread that the
+ * thread_dispatch_ust_registration thread is terminated. Allowing the
+ * ust_thread_manage_notify to perform its termination.
+ */
+int thread_dispatch_ust_registration_teardown_complete_pipe[2] = { -1, -1 };
+
+/*
  * This pipe is used to inform the thread managing application communication
  * that a command is queued and ready to be processed.
  */
@@ -477,6 +484,11 @@ static int init_thread_quit_pipe(void)
 	return __init_thread_quit_pipe(thread_quit_pipe);
 }
 
+static int init_thread_dispatch_ust_registration_teardown_complete_pipe(void)
+{
+	return __init_thread_quit_pipe(thread_dispatch_ust_registration_teardown_complete_pipe);
+}
+
 /*
  * Stop all threads by closing the thread quit pipe.
  */
@@ -623,6 +635,7 @@ static void sessiond_cleanup(void)
 	 * since we are now called.
 	 */
 	utils_close_pipe(thread_quit_pipe);
+	utils_close_pipe(thread_dispatch_ust_registration_teardown_complete_pipe);
 
 	/*
 	 * If opt_pidfile is undefined, the default file will be wiped when
@@ -2128,6 +2141,11 @@ static void *thread_dispatch_ust_registration(void *data)
 	err = 0;
 
 error:
+	ret = notify_thread_pipe(thread_dispatch_ust_registration_teardown_complete_pipe[1]);
+	if (ret < 0) {
+		ERR("write error on thread thread_dispatch_ust_registration_teardown_complete_pipe");
+	}
+
 	/* Clean up wait queue. */
 	cds_list_for_each_entry_safe(wait_node, tmp_wait_node,
 			&wait_queue.head, head) {
@@ -5711,12 +5729,16 @@ int main(int argc, char **argv)
 		goto exit_ht_cleanup;
 	}
 
-	/* Create thread quit pipe */
 	if (init_thread_quit_pipe()) {
 		retval = -1;
 		goto exit_init_data;
 	}
 
+	if (init_thread_dispatch_ust_registration_teardown_complete_pipe()) {
+		retval = -1;
+		goto exit_init_data;
+	}
+
 	/* Check if daemon is UID = 0 */
 	is_root = !getuid();
 
diff --git a/src/bin/lttng-sessiond/ust-thread.c b/src/bin/lttng-sessiond/ust-thread.c
index e605703a..c8d5ffdf 100644
--- a/src/bin/lttng-sessiond/ust-thread.c
+++ b/src/bin/lttng-sessiond/ust-thread.c
@@ -27,6 +27,11 @@
 #include "health-sessiond.h"
 #include "testpoint.h"
 
+static
+int thread_should_quit(int pollfd, uint32_t revents)
+{
+	return (pollfd == thread_dispatch_ust_registration_teardown_complete_pipe[0] && (revents & LPOLLIN)) ? 1 : 0;
+}
 
 static
 void notify_sock_unregister_all()
@@ -64,11 +69,16 @@ void *ust_thread_manage_notify(void *data)
 
 	health_code_update();
 
-	ret = sessiond_set_thread_pollset(&events, 2);
+	ret = lttng_poll_create(&events, 2, LTTNG_CLOEXEC);
 	if (ret < 0) {
 		goto error_poll_create;
 	}
 
+	ret = lttng_poll_add(&events, thread_dispatch_ust_registration_teardown_complete_pipe[0], LPOLLIN | LPOLLERR);
+	if (ret < 0) {
+		goto error;
+	}
+
 	/* Add notify pipe to the pollset. */
 	ret = lttng_poll_add(&events, apps_cmd_notify_pipe[0],
 			LPOLLIN | LPOLLERR | LPOLLHUP | LPOLLRDHUP);
@@ -112,9 +122,7 @@ restart:
 				continue;
 			}
 
-			/* Thread quit pipe has been closed. Killing thread. */
-			ret = sessiond_check_thread_quit_pipe(pollfd, revents);
-			if (ret) {
+			if (thread_should_quit(pollfd, revents)) {
 				err = 0;
 				goto exit;
 			}
-- 
2.11.0