Fix: ambiguous ownership of kernel context by multiple channels

A kernel context, when added to multiple channels, must be copied
before being added to individual channels. The current code
adds the same ltt_kernel_context structure to multiple kernel
channels which introduces a conceptual ambiguity in the ownership
of the context object.

Concretely, creating multiple kernel channels and adding a context
to all of them (by not specifying a channel name) causes the context
to be added to each channels' list of contexts, overwritting the
context's list node, and causing the channel context lists to become
corrupted. This results in crashes being observed during the
destruction of the session.

Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>

diff --git a/src/bin/lttng-sessiond/context.c b/src/bin/lttng-sessiond/context.c
index 9c3a394edbcb7e8e37bb379ebbdbea2e1f915876..a2f022886306ccb55e9c72a4751d896d6c3f9ae2 100644 (file)
--- a/src/bin/lttng-sessiond/context.c
+++ b/src/bin/lttng-sessiond/context.c
@@ -34,6 +34,8 @@
 
 /*
  * Add kernel context to all channel.
+ *
+ * Assumes the ownership of kctx.
  */
 static int add_kctx_all_channels(struct ltt_kernel_session *ksession,
                struct ltt_kernel_context *kctx)
@@ -48,7 +50,17 @@ static int add_kctx_all_channels(struct ltt_kernel_session *ksession,
 
        /* Go over all channels */
        cds_list_for_each_entry(kchan, &ksession->channel_list.head, list) {
-               ret = kernel_add_channel_context(kchan, kctx);
+               struct ltt_kernel_context *kctx_copy;
+
+               kctx_copy = trace_kernel_copy_context(kctx);
+               if (!kctx_copy) {
+                       PERROR("zmalloc ltt_kernel_context");
+                       goto error;
+               }
+
+               /* Ownership of kctx_copy is transferred to the callee. */
+               ret = kernel_add_channel_context(kchan, kctx_copy);
+               kctx_copy = NULL;
                if (ret != 0) {
                        goto error;
                }
@@ -57,11 +69,14 @@ static int add_kctx_all_channels(struct ltt_kernel_session *ksession,
        ret = LTTNG_OK;
 
 error:
+       trace_kernel_destroy_context(kctx);
        return ret;
 }
 
 /*
  * Add kernel context to a specific channel.
+ *
+ * Assumes the ownership of kctx.
  */
 static int add_kctx_to_channel(struct ltt_kernel_context *kctx,
                struct ltt_kernel_channel *kchan)
@@ -73,7 +88,9 @@ static int add_kctx_to_channel(struct ltt_kernel_context *kctx,
 
        DBG("Add kernel context to channel '%s'", kchan->channel->name);
 
+       /* Ownership of kctx is transferred to the callee. */
        ret = kernel_add_channel_context(kchan, kctx);
+       kctx = NULL;
        if (ret != 0) {
                goto error;
        }
@@ -254,6 +271,8 @@ int context_kernel_add(struct ltt_kernel_session *ksession,
 
        if (*channel_name == '\0') {
                ret = add_kctx_all_channels(ksession, kctx);
+               /* Ownership of kctx is transferred to the callee. */
+               kctx = NULL;
                if (ret != LTTNG_OK) {
                        goto error;
                }
@@ -266,12 +285,14 @@ int context_kernel_add(struct ltt_kernel_session *ksession,
                }
 
                ret = add_kctx_to_channel(kctx, kchan);
+               /* Ownership of kctx is transferred to the callee. */
+               kctx = NULL;
                if (ret != LTTNG_OK) {
                        goto error;
                }
        }
 
-       return LTTNG_OK;
+       ret = LTTNG_OK;
 
 error:
        if (kctx) {

diff --git a/src/bin/lttng-sessiond/kernel.c b/src/bin/lttng-sessiond/kernel.c
index abf038f4d59a3f1c9f021123e29e947db3fd4db1..2031bfb38f4ca3825f37067a2ddbca50890e54f8 100644 (file)
--- a/src/bin/lttng-sessiond/kernel.c
+++ b/src/bin/lttng-sessiond/kernel.c
@@ -36,6 +36,8 @@
 
 /*
  * Add context on a kernel channel.
+ *
+ * Assumes the ownership of ctx.
  */
 int kernel_add_channel_context(struct ltt_kernel_channel *chan,
                struct ltt_kernel_context *ctx)
@@ -67,7 +69,11 @@ int kernel_add_channel_context(struct ltt_kernel_channel *chan,
 
 end:
        cds_list_add_tail(&ctx->list, &chan->ctx_list);
+       ctx = NULL;
 error:
+       if (ctx) {
+               trace_kernel_destroy_context(ctx);
+       }
        return ret;
 }
 

diff --git a/src/bin/lttng-sessiond/trace-kernel.c b/src/bin/lttng-sessiond/trace-kernel.c
index 083add3f30f907eee80dc259b0e961f028e50ab3..54bdc667575ec48a0b2d5ac3fadcc999148150ae 100644 (file)
--- a/src/bin/lttng-sessiond/trace-kernel.c
+++ b/src/bin/lttng-sessiond/trace-kernel.c
@@ -257,11 +257,33 @@ struct ltt_kernel_context *trace_kernel_create_context(
        if (ctx) {
                memcpy(&kctx->ctx, ctx, sizeof(kctx->ctx));
        }
+error:
+       return kctx;
+}
 
-       CDS_INIT_LIST_HEAD(&kctx->list);
+/*
+ * Allocate and init a kernel context object from an existing kernel context
+ * object.
+ *
+ * Return the allocated object or NULL on error.
+ */
+struct ltt_kernel_context *trace_kernel_copy_context(
+               struct ltt_kernel_context *kctx)
+{
+       struct ltt_kernel_context *kctx_copy;
+
+       assert(kctx);
+       kctx_copy = zmalloc(sizeof(*kctx_copy));
+       if (!kctx_copy) {
+               PERROR("zmalloc ltt_kernel_context");
+               goto error;
+       }
+
+       memcpy(kctx_copy, kctx, sizeof(*kctx_copy));
+       memset(&kctx_copy->list, 0, sizeof(kctx_copy->list));
 
 error:
-       return kctx;
+       return kctx_copy;
 }
 
 /*

diff --git a/src/bin/lttng-sessiond/trace-kernel.h b/src/bin/lttng-sessiond/trace-kernel.h
index 5879ca281be62b29b490d54e86426066934bed83..0ac020f97d63b4d00d599ec2ee2597a8bbe44d2f 100644 (file)
--- a/src/bin/lttng-sessiond/trace-kernel.h
+++ b/src/bin/lttng-sessiond/trace-kernel.h
@@ -143,6 +143,8 @@ struct ltt_kernel_stream *trace_kernel_create_stream(const char *name,
                unsigned int count);
 struct ltt_kernel_context *trace_kernel_create_context(
                struct lttng_kernel_context *ctx);
+struct ltt_kernel_context *trace_kernel_copy_context(
+               struct ltt_kernel_context *ctx);
 
 /*
  * Destroy functions free() the data structure and remove from linked list if