Fix: metadata stream should not reference session

The metadata stream should only reference the metadata cache, not the
session. Otherwise, we end up in a catch 22 situation:

- Stream POLLHUP is only given when the session is destroyed, but,
- The session is only destroyed when all references to session are
  released, including references from channels, but,
- If the metadata stream holds a reference on the metadata session, we
  end up with a circular dependency loop.

Fix this by making sure the metadata stream does not use any of the
lttng channel nor lttng session.

Reviewed-by: Julien Desfossez <jdesfossez@efficios.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

diff --git a/lib/ringbuffer/ring_buffer_vfs.c b/lib/ringbuffer/ring_buffer_vfs.c
index f15b974be0e48c2a95fd77ddfe39647e93382b0b..dae73de398c5ea38dba90f0bd384d5f21f327d30 100644 (file)
--- a/lib/ringbuffer/ring_buffer_vfs.c
+++ b/lib/ringbuffer/ring_buffer_vfs.c
@@ -41,6 +41,10 @@ static int compat_put_ulong(compat_ulong_t val, unsigned long arg)
 }
 #endif
 
+/*
+ * This is not used by anonymous file descriptors. This code is left
+ * there if we ever want to implement an inode with open() operation.
+ */
 int lib_ring_buffer_open(struct inode *inode, struct file *file,
                struct lib_ring_buffer *buf)
 {

diff --git a/lttng-abi.c b/lttng-abi.c
index 26a456ece13f7ed0a62b5e3b3e92d2cff942c17c..1be6802a0f3c511677048e578476b71785e113d4 100644 (file)
--- a/lttng-abi.c
+++ b/lttng-abi.c
@@ -553,10 +553,9 @@ int lttng_metadata_ring_buffer_ioctl_get_next_subbuf(struct file *filp,
        struct lttng_metadata_stream *stream = filp->private_data;
        struct lib_ring_buffer *buf = stream->priv;
        struct channel *chan = buf->backend.chan;
-       struct lttng_channel *lttng_chan = channel_get_private(chan);
        int ret;
 
-       ret = lttng_metadata_output_channel(lttng_chan, stream);
+       ret = lttng_metadata_output_channel(stream, chan);
        if (ret > 0) {
                lib_ring_buffer_switch_slow(buf, SWITCH_ACTIVE);
                ret = 0;
@@ -671,6 +670,10 @@ err:
 }
 #endif
 
+/*
+ * This is not used by anonymous file descriptors. This code is left
+ * there if we ever want to implement an inode with open() operation.
+ */
 static
 int lttng_metadata_ring_buffer_open(struct inode *inode, struct file *file)
 {
@@ -678,6 +681,14 @@ int lttng_metadata_ring_buffer_open(struct inode *inode, struct file *file)
        struct lib_ring_buffer *buf = stream->priv;
 
        file->private_data = buf;
+       /*
+        * Since life-time of metadata cache differs from that of
+        * session, we need to keep our own reference on the transport.
+        */
+       if (!try_module_get(stream->transport->owner)) {
+               printk(KERN_WARNING "LTT : Can't lock transport module.\n");
+               return -EBUSY;
+       }
        return lib_ring_buffer_open(inode, file, buf);
 }
 
@@ -686,12 +697,9 @@ int lttng_metadata_ring_buffer_release(struct inode *inode, struct file *file)
 {
        struct lttng_metadata_stream *stream = file->private_data;
        struct lib_ring_buffer *buf = stream->priv;
-       struct channel *chan = buf->backend.chan;
-       struct lttng_channel *lttng_chan = channel_get_private(chan);
 
        kref_put(&stream->metadata_cache->refcount, metadata_cache_destroy);
-       fput(lttng_chan->file);
-
+       module_put(stream->transport->owner);
        return lib_ring_buffer_release(inode, file, buf);
 }
 
@@ -811,24 +819,41 @@ int lttng_abi_open_metadata_stream(struct file *channel_file)
 
        metadata_stream = kzalloc(sizeof(struct lttng_metadata_stream),
                        GFP_KERNEL);
-       if (!metadata_stream)
-               return -ENOMEM;
+       if (!metadata_stream) {
+               ret = -ENOMEM;
+               goto nomem;
+       }
        metadata_stream->metadata_cache = session->metadata_cache;
        init_waitqueue_head(&metadata_stream->read_wait);
        metadata_stream->priv = buf;
        stream_priv = metadata_stream;
+       metadata_stream->transport = channel->transport;
+
+       /*
+        * Since life-time of metadata cache differs from that of
+        * session, we need to keep our own reference on the transport.
+        */
+       if (!try_module_get(metadata_stream->transport->owner)) {
+               printk(KERN_WARNING "LTT : Can't lock transport module.\n");
+               ret = -EINVAL;
+               goto notransport;
+       }
+
        ret = lttng_abi_create_stream_fd(channel_file, stream_priv,
                        &lttng_metadata_ring_buffer_file_operations);
        if (ret < 0)
                goto fd_error;
 
-       atomic_long_inc(&channel_file->f_count);
        kref_get(&session->metadata_cache->refcount);
        list_add(&metadata_stream->list,
                &session->metadata_cache->metadata_stream);
        return ret;
 
 fd_error:
+       module_put(metadata_stream->transport->owner);
+notransport:
+       kfree(metadata_stream);
+nomem:
        channel->ops->buffer_read_close(buf);
        return ret;
 }

diff --git a/lttng-events.c b/lttng-events.c
index 567df65acea12345f51e46c937efa5f95cb15202..4b891cd5ecda22e8778f5a4592f19c9e2574172f 100644 (file)
--- a/lttng-events.c
+++ b/lttng-events.c
@@ -554,8 +554,8 @@ void _lttng_event_destroy(struct lttng_event *event)
  * remaining space left in packet and write, since mutual exclusion
  * protects us from concurrent writes.
  */
-int lttng_metadata_output_channel(struct lttng_channel *chan,
-               struct lttng_metadata_stream *stream)
+int lttng_metadata_output_channel(struct lttng_metadata_stream *stream,
+               struct channel *chan)
 {
        struct lib_ring_buffer_ctx ctx;
        int ret = 0;
@@ -574,22 +574,22 @@ int lttng_metadata_output_channel(struct lttng_channel *chan,
        if (!len)
                return 0;
        reserve_len = min_t(size_t,
-                       chan->ops->packet_avail_size(chan->chan),
+                       stream->transport->ops.packet_avail_size(chan),
                        len);
-       lib_ring_buffer_ctx_init(&ctx, chan->chan, NULL, reserve_len,
+       lib_ring_buffer_ctx_init(&ctx, chan, NULL, reserve_len,
                        sizeof(char), -1);
        /*
         * If reservation failed, return an error to the caller.
         */
-       ret = chan->ops->event_reserve(&ctx, 0);
+       ret = stream->transport->ops.event_reserve(&ctx, 0);
        if (ret != 0) {
                printk(KERN_WARNING "LTTng: Metadata event reservation failed\n");
                goto end;
        }
-       chan->ops->event_write(&ctx,
+       stream->transport->ops.event_write(&ctx,
                        stream->metadata_cache->data + stream->metadata_in,
                        reserve_len);
-       chan->ops->event_commit(&ctx);
+       stream->transport->ops.event_commit(&ctx);
        stream->metadata_in += reserve_len;
        ret = reserve_len;
 

diff --git a/lttng-events.h b/lttng-events.h
index 4c1f32288752e092612b791061006654ffcc66d9..bc5cd9f5271b2b088decd100788cc7edf2e9ddeb 100644 (file)
--- a/lttng-events.h
+++ b/lttng-events.h
@@ -283,6 +283,7 @@ struct lttng_metadata_stream {
        int finalized;                  /* Has channel been finalized */
        wait_queue_head_t read_wait;    /* Reader buffer-level wait queue */
        struct list_head list;          /* Stream list */
+       struct lttng_transport *transport;
 };
 
 struct lttng_session {
@@ -356,8 +357,8 @@ void lttng_event_put(const struct lttng_event_desc *desc);
 int lttng_probes_init(void);
 void lttng_probes_exit(void);
 
-int lttng_metadata_output_channel(struct lttng_channel *chan,
-               struct lttng_metadata_stream *stream);
+int lttng_metadata_output_channel(struct lttng_metadata_stream *stream,
+               struct channel *chan);
 
 #if defined(CONFIG_HAVE_SYSCALL_TRACEPOINTS)
 int lttng_syscalls_register(struct lttng_channel *chan, void *filter);