From b3b8072b02b4f1917a0254577c48301e8f44c210 Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Tue, 3 Sep 2013 17:23:15 -0400 Subject: [PATCH] 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 Signed-off-by: Mathieu Desnoyers --- lib/ringbuffer/ring_buffer_vfs.c | 4 +++ lttng-abi.c | 43 +++++++++++++++++++++++++------- lttng-events.c | 14 +++++------ lttng-events.h | 5 ++-- 4 files changed, 48 insertions(+), 18 deletions(-) diff --git a/lib/ringbuffer/ring_buffer_vfs.c b/lib/ringbuffer/ring_buffer_vfs.c index f15b974b..dae73de3 100644 --- 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 26a456ec..1be6802a 100644 --- 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, <tng_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 567df65a..4b891cd5 100644 --- 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 4c1f3228..bc5cd9f5 100644 --- 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); -- 2.34.1