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 e478499336896f5d739e087c8ee91e3a395886cd..fb6f96d47f4050edd28b4eb8507aa55993764800 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)
@@ -66,9 +68,11 @@ int kernel_add_channel_context(struct ltt_kernel_channel *chan,
 
 end:
        cds_list_add_tail(&ctx->list, &chan->ctx_list);
-       return 0;
-
+       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 29432de49a0355b16863ab69189929a6c37719f3..ce5aca2edfb376d55563253d888d9afd0397e584 100644 (file)
--- a/src/bin/lttng-sessiond/trace-kernel.c
+++ b/src/bin/lttng-sessiond/trace-kernel.c
@@ -240,11 +240,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 93870cbfd6509f98e92b3ff4a4bbb5638c09aaf2..a39ccd3bb0b6d0b5fd24d9f73cf3d9764623c3f2 100644 (file)
--- a/src/bin/lttng-sessiond/trace-kernel.h
+++ b/src/bin/lttng-sessiond/trace-kernel.h
@@ -142,6 +142,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