Fix: hash table cleanup call_rcu deadlock
[lttng-tools.git] / src / bin / lttng-sessiond / ust-app.c
index d8c8017d83800511afa90e7223211a002e7d6481..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
@@ -2183,6 +2216,7 @@ static int send_channel_uid_to_ust(struct buffer_reg_channel *reg_chan,
 
                ret = ust_consumer_send_stream_to_ust(app, ua_chan, &stream);
                if (ret < 0) {
+                       (void) release_ust_app_stream(-1, &stream);
                        goto error_stream_unlock;
                }
 
@@ -2245,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;
                }
 
@@ -2403,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);
 
@@ -2431,6 +2473,7 @@ end:
 
 error:
        delete_ust_app_channel(ua_chan->is_sent ? app->sock : -1, ua_chan, app);
+error_alloc:
        return ret;
 }
 
@@ -2507,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;
        }
@@ -2526,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 {
@@ -2536,13 +2579,6 @@ static int create_ust_app_metadata(struct ust_app_session *ua_sess,
                metadata->attr.type = LTTNG_UST_CHAN_METADATA;
        }
 
-       /* Get the right consumer socket for the application. */
-       socket = consumer_find_socket_by_bitness(app->bits_per_long, consumer);
-       if (!socket) {
-               ret = -EINVAL;
-               goto error_consumer;
-       }
-
        /* Need one fd for the channel. */
        ret = lttng_fd_get(LTTNG_FD_APPS, 1);
        if (ret < 0) {
@@ -2550,6 +2586,13 @@ static int create_ust_app_metadata(struct ust_app_session *ua_sess,
                goto error;
        }
 
+       /* Get the right consumer socket for the application. */
+       socket = consumer_find_socket_by_bitness(app->bits_per_long, consumer);
+       if (!socket) {
+               ret = -EINVAL;
+               goto error_consumer;
+       }
+
        /*
         * Keep metadata key so we can identify it on the consumer side. Assign it
         * to the registry *before* we ask the consumer so we avoid the race of the
@@ -2571,7 +2614,6 @@ static int create_ust_app_metadata(struct ust_app_session *ua_sess,
                 * Safe because the metadata obj pointer is not set so the delete below
                 * will not put a FD back again.
                 */
-               lttng_fd_put(LTTNG_FD_APPS, 1);
                goto error_consumer;
        }
 
@@ -2587,7 +2629,6 @@ static int create_ust_app_metadata(struct ust_app_session *ua_sess,
                 * Safe because the metadata obj pointer is not set so the delete below
                 * will not put a FD back again.
                 */
-               lttng_fd_put(LTTNG_FD_APPS, 1);
                goto error_consumer;
        }
 
@@ -2595,6 +2636,7 @@ static int create_ust_app_metadata(struct ust_app_session *ua_sess,
                        metadata->key, app->pid);
 
 error_consumer:
+       lttng_fd_put(LTTNG_FD_APPS, 1);
        delete_ust_app_channel(-1, metadata, app);
 error:
        return ret;
@@ -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.029354 seconds and 4 git commands to generate.