Fix: take the ust lock around session iteration in statedump

This is crafted _very_ carefully: we need to always take the ust lock
_within_ the dynamic loader lock.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

diff --git a/include/lttng/ust-events.h b/include/lttng/ust-events.h
index 28f7391b56a5144bae513d67dabab498edb472d7..d93c912165e1f72174d17c0394831a4caca25e6a 100644 (file)
--- a/include/lttng/ust-events.h
+++ b/include/lttng/ust-events.h
@@ -610,6 +610,7 @@ struct cds_list_head *lttng_get_probe_list_head(void);
 int lttng_session_active(void);
 
 typedef int (*t_statedump_func_ptr)(struct lttng_session *session);
-int lttng_handle_pending_statedumps(t_statedump_func_ptr statedump_func_ptr);
+int lttng_handle_pending_statedump(void *owner);
+struct cds_list_head *_lttng_get_sessions(void);
 
 #endif /* _LTTNG_UST_EVENTS_H */

diff --git a/liblttng-ust/lttng-events.c b/liblttng-ust/lttng-events.c
index 9380e9ceb688b9b46df58122d7cc3a47c14421b3..6e16cf2548f04087ec3d1b6805736d3da1f179b5 100644 (file)
--- a/liblttng-ust/lttng-events.c
+++ b/liblttng-ust/lttng-events.c
@@ -54,6 +54,7 @@
 #include "tracepoint-internal.h"
 #include "lttng-tracer.h"
 #include "lttng-tracer-core.h"
+#include "lttng-ust-baddr.h"
 #include "wait.h"
 #include "../libringbuffer/shm.h"
 #include "jhash.h"
@@ -77,6 +78,11 @@ void ust_unlock(void)
 
 static CDS_LIST_HEAD(sessions);
 
+struct cds_list_head *_lttng_get_sessions(void)
+{
+       return &sessions;
+}
+
 static void _lttng_event_destroy(struct lttng_event *event);
 
 static
@@ -676,18 +682,27 @@ int lttng_fix_pending_events(void)
 }
 
 /*
- * Called after session enable: For each session, execute pending statedumps.
+ * For each session of the owner thread, execute pending statedump.
+ * Only dump state for the sessions owned by the caller thread, because
+ * we don't keep ust_lock across the entire iteration.
  */
-int lttng_handle_pending_statedumps(t_statedump_func_ptr statedump_func_ptr)
+int lttng_handle_pending_statedump(void *owner)
 {
        struct lttng_session *session;
 
+       /* Execute state dump */
+       lttng_ust_baddr_statedump(owner);
+
+       /* Clear pending state dump */
+       ust_lock();
        cds_list_for_each_entry(session, &sessions, node) {
-               if (session->statedump_pending) {
-                       session->statedump_pending = 0;
-                       statedump_func_ptr(session);
-               }
+               if (session->owner != owner)
+                       continue;
+               if (!session->statedump_pending)
+                       continue;
+               session->statedump_pending = 0;
        }
+       ust_unlock();
        return 0;
 }
 

diff --git a/liblttng-ust/lttng-ust-baddr.c b/liblttng-ust/lttng-ust-baddr.c
index 1e6e08dc6343627a2a261dca4619798b7993915b..001103a283eb32c1f6e5b6c48ad5107ed3b60a1f 100644 (file)
--- a/liblttng-ust/lttng-ust-baddr.c
+++ b/liblttng-ust/lttng-ust-baddr.c
@@ -36,16 +36,21 @@
 #define TRACEPOINT_DEFINE
 #include "ust_baddr_statedump.h"
 
-static int
-extract_soinfo_events(struct dl_phdr_info *info, size_t size, void *data)
+static
+int extract_soinfo_events(struct dl_phdr_info *info, size_t size, void *data)
 {
        int j;
        int num_loadable_segment = 0;
+       void *owner = data;
+       struct cds_list_head *sessionsp;
+
+       sessionsp = _lttng_get_sessions();
 
        for (j = 0; j < info->dlpi_phnum; j++) {
                char resolved_path[PATH_MAX];
                struct stat sostat;
                void *base_addr_ptr;
+               struct lttng_session *session;
 
                if (info->dlpi_phdr[j].p_type != PT_LOAD)
                        continue;
@@ -83,9 +88,22 @@ extract_soinfo_events(struct dl_phdr_info *info, size_t size, void *data)
                        sostat.st_mtime = -1;
                }
 
-               tracepoint(ust_baddr_statedump, soinfo,
-                               (struct lttng_session *) data, base_addr_ptr,
-                               resolved_path, sostat.st_size, sostat.st_mtime);
+               /*
+                * UST lock needs to be nested within dynamic loader
+                * lock.
+                */
+               ust_lock();
+               cds_list_for_each_entry(session, sessionsp, node) {
+                       if (session->owner != owner)
+                               continue;
+                       if (!session->statedump_pending)
+                               continue;
+                       tracepoint(ust_baddr_statedump, soinfo,
+                                       session, base_addr_ptr,
+                                       resolved_path, sostat.st_size,
+                                       sostat.st_mtime);
+               }
+               ust_unlock();
 
                /*
                 * We are only interested in the base address (lowest virtual
@@ -96,15 +114,15 @@ extract_soinfo_events(struct dl_phdr_info *info, size_t size, void *data)
        return 0;
 }
 
-int
-lttng_ust_baddr_statedump(struct lttng_session *session)
+int lttng_ust_baddr_statedump(void *owner)
 {
        if (getenv("LTTNG_UST_WITHOUT_BADDR_STATEDUMP"))
                return 0;
        /*
         * Iterate through the list of currently loaded shared objects and
-        * generate events for loadable segments using extract_soinfo_events
+        * generate events for loadable segments using
+        * extract_soinfo_events.
         */
-       dl_iterate_phdr(extract_soinfo_events, session);
+       dl_iterate_phdr(extract_soinfo_events, owner);
        return 0;
 }

diff --git a/liblttng-ust/lttng-ust-baddr.h b/liblttng-ust/lttng-ust-baddr.h
index d338541ab5793755cb18f49d7de41eb692c49810..08f7db7d0dd960cf76c6ea9d42d893357a725021 100644 (file)
--- a/liblttng-ust/lttng-ust-baddr.h
+++ b/liblttng-ust/lttng-ust-baddr.h
@@ -21,6 +21,6 @@
 
 #include <lttng/ust-events.h>
 
-int lttng_ust_baddr_statedump(struct lttng_session *session);
+int lttng_ust_baddr_statedump(void *owner);
 
 #endif /* LTTNG_UST_BADDR_H */

diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c
index 4724fabc0d6a72cf57923cea7e6da44d94563052..256cbdc625ef19f6da306124542a6e0a08ab593f 100644 (file)
--- a/liblttng-ust/lttng-ust-comm.c
+++ b/liblttng-ust/lttng-ust-comm.c
@@ -108,7 +108,8 @@ struct sock_info {
 
        char wait_shm_path[PATH_MAX];
        char *wait_shm_mmap;
-       int session_enabled;
+       /* Keep track of lazy state dump not performed yet. */
+       int statedump_pending;
 };
 
 /* Socket from app (connect) to session daemon (listen) for communication */
@@ -126,7 +127,7 @@ struct sock_info global_apps = {
 
        .wait_shm_path = "/" LTTNG_UST_WAIT_FILENAME,
 
-       .session_enabled = 0,
+       .statedump_pending = 0,
 };
 
 /* TODO: allow global_apps_sock_path override */
@@ -141,7 +142,7 @@ struct sock_info local_apps = {
        .socket = -1,
        .notify_socket = -1,
 
-       .session_enabled = 0,
+       .statedump_pending = 0,
 };
 
 static int wait_poll_fallback;
@@ -387,6 +388,28 @@ int handle_register_done(struct sock_info *sock_info)
        return 0;
 }
 
+/*
+ * Only execute pending statedump after the constructor semaphore has
+ * been posted by each listener thread. This means statedump will only
+ * be performed after the "registration done" command is received from
+ * each session daemon the application is connected to.
+ *
+ * This ensures we don't run into deadlock issues with the dynamic
+ * loader mutex, which is held while the constructor is called and
+ * waiting on the constructor semaphore. All operations requiring this
+ * dynamic loader lock need to be postponed using this mechanism.
+ */
+static
+void handle_pending_statedump(struct sock_info *sock_info)
+{
+       int ctor_passed = sock_info->constructor_sem_posted;
+
+       if (ctor_passed && sock_info->statedump_pending) {
+               sock_info->statedump_pending = 0;
+               lttng_handle_pending_statedump(sock_info);
+       }
+}
+
 static
 int handle_message(struct sock_info *sock_info,
                int sock, struct ustcomm_ust_msg *lum)
@@ -705,18 +728,15 @@ end:
 
 error:
        ust_unlock();
-       return ret;
-}
 
-static
-void handle_pending_statedumps(struct sock_info *sock_info)
-{
-       int ctor_passed = sock_info->constructor_sem_posted;
+       /*
+        * Performed delayed statedump operations outside of the UST
+        * lock. We need to take the dynamic loader lock before we take
+        * the UST lock internally within handle_pending_statedump().
+         */
+       handle_pending_statedump(sock_info);
 
-       if (ctor_passed && sock_info->session_enabled) {
-               sock_info->session_enabled = 0;
-               lttng_handle_pending_statedumps(&lttng_ust_baddr_statedump);
-       }
+       return ret;
 }
 
 static
@@ -1224,8 +1244,6 @@ restart:
                        ret = handle_message(sock_info, sock, &lum);
                        if (ret) {
                                ERR("Error handling message for %s socket", sock_info->name);
-                       } else {
-                               handle_pending_statedumps(sock_info);
                        }
                        continue;
                default:
@@ -1543,5 +1561,5 @@ void ust_after_fork_child(sigset_t *restore_sigset)
 void lttng_ust_sockinfo_session_enabled(void *owner)
 {
        struct sock_info *sock_info = owner;
-       sock_info->session_enabled = 1;
+       sock_info->statedump_pending = 1;
 }