Fix: hash table cleanup call_rcu deadlock
[lttng-tools.git] / src / bin / lttng-sessiond / ust-app.c
index e36238182d8f04e078d440dce4d3bee94b9c5360..37f6442156ff03a8724ef62137e66b1548544f30 100644 (file)
@@ -38,6 +38,7 @@
 #include "ust-app.h"
 #include "ust-consumer.h"
 #include "ust-ctl.h"
+#include "utils.h"
 
 /* Next available channel key. */
 static unsigned long next_channel_key;
@@ -307,9 +308,9 @@ void delete_ust_app_stream(int sock, struct ust_app_stream *stream)
 
 /*
  * We need to execute ht_destroy outside of RCU read-side critical
- * section, so we postpone its execution using call_rcu. It is simpler
- * than to change the semantic of the many callers of
- * delete_ust_app_channel().
+ * section and outside of call_rcu thread, so we postpone its execution
+ * using ht_cleanup_push. It is simpler than to change the semantic of
+ * the many callers of delete_ust_app_session().
  */
 static
 void delete_ust_app_channel_rcu(struct rcu_head *head)
@@ -317,8 +318,8 @@ void delete_ust_app_channel_rcu(struct rcu_head *head)
        struct ust_app_channel *ua_chan =
                caa_container_of(head, struct ust_app_channel, rcu_head);
 
-       lttng_ht_destroy(ua_chan->ctx);
-       lttng_ht_destroy(ua_chan->events);
+       ht_cleanup_push(ua_chan->ctx);
+       ht_cleanup_push(ua_chan->events);
        free(ua_chan);
 }
 
@@ -384,7 +385,10 @@ void delete_ust_app_channel(int sock, struct ust_app_channel *ua_chan,
 }
 
 /*
- * Push metadata to consumer socket. The socket lock MUST be acquired.
+ * Push metadata to consumer socket.
+ *
+ * The socket lock MUST be acquired.
+ * The ust app session lock MUST be acquired.
  *
  * On success, return the len of metadata pushed or else a negative value.
  */
@@ -398,8 +402,19 @@ ssize_t ust_app_push_metadata(struct ust_registry_session *registry,
 
        assert(registry);
        assert(socket);
-       /* Should never be 0 which is the initial state. */
-       assert(registry->metadata_key);
+
+       /*
+        * On a push metadata error either the consumer is dead or the metadata
+        * channel has been destroyed because its endpoint might have died (e.g:
+        * relayd). If so, the metadata closed flag is set to 1 so we deny pushing
+        * metadata again which is not valid anymore on the consumer side.
+        *
+        * The ust app session mutex locked allows us to make this check without
+        * the registry lock.
+        */
+       if (registry->metadata_closed) {
+               return -EPIPE;
+       }
 
        pthread_mutex_lock(&registry->lock);
 
@@ -474,7 +489,7 @@ static int push_metadata(struct ust_registry_session *registry,
         */
        if (!registry->metadata_key) {
                ret_val = 0;
-               goto error_rcu_unlock;
+               goto end_rcu_unlock;
        }
 
        /* Get consumer socket to use to push the metadata.*/
@@ -507,6 +522,13 @@ static int push_metadata(struct ust_registry_session *registry,
        return 0;
 
 error_rcu_unlock:
+       /*
+        * On error, flag the registry that the metadata is closed. We were unable
+        * to push anything and this means that either the consumer is not
+        * responding or the metadata cache has been destroyed on the consumer.
+        */
+       registry->metadata_closed = 1;
+end_rcu_unlock:
        rcu_read_unlock();
        return ret_val;
 }
@@ -532,7 +554,7 @@ static int close_metadata(struct ust_registry_session *registry,
 
        if (!registry->metadata_key || registry->metadata_closed) {
                ret = 0;
-               goto error;
+               goto end;
        }
 
        /* Get consumer socket to use to push the metadata.*/
@@ -548,19 +570,23 @@ static int close_metadata(struct ust_registry_session *registry,
                goto error;
        }
 
-       /* Metadata successfully closed. Flag the registry. */
-       registry->metadata_closed = 1;
-
 error:
+       /*
+        * Metadata closed. Even on error this means that the consumer is not
+        * responding or not found so either way a second close should NOT be emit
+        * for this registry.
+        */
+       registry->metadata_closed = 1;
+end:
        rcu_read_unlock();
        return ret;
 }
 
 /*
  * We need to execute ht_destroy outside of RCU read-side critical
- * section, so we postpone its execution using call_rcu. It is simpler
- * than to change the semantic of the many callers of
- * delete_ust_app_session().
+ * section and outside of call_rcu thread, so we postpone its execution
+ * using ht_cleanup_push. It is simpler than to change the semantic of
+ * the many callers of delete_ust_app_session().
  */
 static
 void delete_ust_app_session_rcu(struct rcu_head *head)
@@ -568,7 +594,7 @@ void delete_ust_app_session_rcu(struct rcu_head *head)
        struct ust_app_session *ua_sess =
                caa_container_of(head, struct ust_app_session, rcu_head);
 
-       lttng_ht_destroy(ua_sess->channels);
+       ht_cleanup_push(ua_sess->channels);
        free(ua_sess);
 }
 
@@ -587,16 +613,21 @@ void delete_ust_app_session(int sock, struct ust_app_session *ua_sess,
 
        assert(ua_sess);
 
+       pthread_mutex_lock(&ua_sess->lock);
+
        registry = get_session_registry(ua_sess);
-       if (registry) {
+       if (registry && !registry->metadata_closed) {
                /* Push metadata for application before freeing the application. */
                (void) push_metadata(registry, ua_sess->consumer);
 
                /*
                 * Don't ask to close metadata for global per UID buffers. Close
-                * metadata only on destroy trace session in this case.
+                * metadata only on destroy trace session in this case. Also, the
+                * previous push metadata could have flag the metadata registry to
+                * close so don't send a close command if closed.
                 */
-               if (ua_sess->buffer_type != LTTNG_BUFFER_PER_UID) {
+               if (ua_sess->buffer_type != LTTNG_BUFFER_PER_UID &&
+                               !registry->metadata_closed) {
                        /* And ask to close it for this session registry. */
                        (void) close_metadata(registry, ua_sess->consumer);
                }
@@ -625,6 +656,8 @@ void delete_ust_app_session(int sock, struct ust_app_session *ua_sess,
                                        sock, ret);
                }
        }
+       pthread_mutex_unlock(&ua_sess->lock);
+
        call_rcu(&ua_sess->rcu_head, delete_ust_app_session_rcu);
 }
 
@@ -653,8 +686,8 @@ void delete_ust_app(struct ust_app *app)
                rcu_read_unlock();
        }
 
-       lttng_ht_destroy(app->sessions);
-       lttng_ht_destroy(app->ust_objd);
+       ht_cleanup_push(app->sessions);
+       ht_cleanup_push(app->ust_objd);
 
        /*
         * Wait until we have deleted the application from the sock hash table
@@ -2246,6 +2279,14 @@ static int create_channel_per_uid(struct ust_app *app,
                ret = do_consumer_create_channel(usess, ua_sess, ua_chan,
                                app->bits_per_long, reg_uid->registry->reg.ust);
                if (ret < 0) {
+                       /*
+                        * Let's remove the previously created buffer registry channel so
+                        * it's not visible anymore in the session registry.
+                        */
+                       ust_registry_channel_del_free(reg_uid->registry->reg.ust,
+                                       ua_chan->tracing_channel_id);
+                       buffer_reg_channel_remove(reg_uid->registry, reg_chan);
+                       buffer_reg_channel_destroy(reg_chan, LTTNG_DOMAIN_UST);
                        goto error;
                }
 
@@ -2404,7 +2445,7 @@ static int create_ust_app_channel(struct ust_app_session *ua_sess,
        if (ua_chan == NULL) {
                /* Only malloc can fail here */
                ret = -ENOMEM;
-               goto error;
+               goto error_alloc;
        }
        shadow_copy_channel(ua_chan, uchan);
 
@@ -2432,6 +2473,7 @@ end:
 
 error:
        delete_ust_app_channel(ua_chan->is_sent ? app->sock : -1, ua_chan, app);
+error_alloc:
        return ret;
 }
 
@@ -2508,8 +2550,8 @@ static int create_ust_app_metadata(struct ust_app_session *ua_sess,
        registry = get_session_registry(ua_sess);
        assert(registry);
 
-       /* Metadata already exists for this registry. */
-       if (registry->metadata_key) {
+       /* Metadata already exists for this registry or it was closed previously */
+       if (registry->metadata_key || registry->metadata_closed) {
                ret = 0;
                goto error;
        }
@@ -2527,8 +2569,8 @@ static int create_ust_app_metadata(struct ust_app_session *ua_sess,
                metadata->attr.overwrite = DEFAULT_CHANNEL_OVERWRITE;
                metadata->attr.subbuf_size = default_get_metadata_subbuf_size();
                metadata->attr.num_subbuf = DEFAULT_METADATA_SUBBUF_NUM;
-               metadata->attr.switch_timer_interval = DEFAULT_UST_CHANNEL_SWITCH_TIMER;
-               metadata->attr.read_timer_interval = DEFAULT_UST_CHANNEL_READ_TIMER;
+               metadata->attr.switch_timer_interval = DEFAULT_METADATA_SWITCH_TIMER;
+               metadata->attr.read_timer_interval = DEFAULT_METADATA_READ_TIMER;
                metadata->attr.output = LTTNG_UST_MMAP;
                metadata->attr.type = LTTNG_UST_CHAN_METADATA;
        } else {
@@ -2842,15 +2884,18 @@ void ust_app_unregister(int sock)
                 * session so the delete session will NOT push/close a second time.
                 */
                registry = get_session_registry(ua_sess);
-               if (registry) {
+               if (registry && !registry->metadata_closed) {
                        /* Push metadata for application before freeing the application. */
                        (void) push_metadata(registry, ua_sess->consumer);
 
                        /*
                         * Don't ask to close metadata for global per UID buffers. Close
-                        * metadata only on destroy trace session in this case.
+                        * metadata only on destroy trace session in this case. Also, the
+                        * previous push metadata could have flag the metadata registry to
+                        * close so don't send a close command if closed.
                         */
-                       if (ua_sess->buffer_type != LTTNG_BUFFER_PER_UID) {
+                       if (ua_sess->buffer_type != LTTNG_BUFFER_PER_UID &&
+                                       !registry->metadata_closed) {
                                /* And ask to close it for this session registry. */
                                (void) close_metadata(registry, ua_sess->consumer);
                        }
@@ -3111,9 +3156,9 @@ void ust_app_clean_list(void)
        rcu_read_unlock();
 
        /* Destroy is done only when the ht is empty */
-       lttng_ht_destroy(ust_app_ht);
-       lttng_ht_destroy(ust_app_ht_by_sock);
-       lttng_ht_destroy(ust_app_ht_by_notify_sock);
+       ht_cleanup_push(ust_app_ht);
+       ht_cleanup_push(ust_app_ht_by_sock);
+       ht_cleanup_push(ust_app_ht_by_notify_sock);
 }
 
 /*
@@ -3733,8 +3778,11 @@ int ust_app_stop_trace(struct ltt_ust_session *usess, struct ust_app *app)
 
        registry = get_session_registry(ua_sess);
        assert(registry);
-       /* Push metadata for application before freeing the application. */
-       (void) push_metadata(registry, ua_sess->consumer);
+
+       if (!registry->metadata_closed) {
+               /* Push metadata for application before freeing the application. */
+               (void) push_metadata(registry, ua_sess->consumer);
+       }
 
        pthread_mutex_unlock(&ua_sess->lock);
 end_no_session:
@@ -4428,9 +4476,14 @@ static int reply_ust_register_channel(int sock, int sobjd, int cobjd,
                goto error_rcu_unlock;
        }
 
-       /* Lookup channel by UST object descriptor. Should always be found. */
+       /* Lookup channel by UST object descriptor. */
        ua_chan = find_channel_by_objd(app, cobjd);
-       assert(ua_chan);
+       if (!ua_chan) {
+               DBG("Application channel is being teardown. Abort event notify");
+               ret = 0;
+               goto error_rcu_unlock;
+       }
+
        assert(ua_chan->session);
        ua_sess = ua_chan->session;
 
@@ -4534,9 +4587,14 @@ static int add_event_ust_registry(int sock, int sobjd, int cobjd, char *name,
                goto error_rcu_unlock;
        }
 
-       /* Lookup channel by UST object descriptor. Should always be found. */
+       /* Lookup channel by UST object descriptor. */
        ua_chan = find_channel_by_objd(app, cobjd);
-       assert(ua_chan);
+       if (!ua_chan) {
+               DBG("Application channel is being teardown. Abort event notify");
+               ret = 0;
+               goto error_rcu_unlock;
+       }
+
        assert(ua_chan->session);
        ua_sess = ua_chan->session;
 
This page took 0.031014 seconds and 4 git commands to generate.