From e1f3997aa650554880105d787a864653add7c070 Mon Sep 17 00:00:00 2001 From: Julien Desfossez Date: Thu, 30 Nov 2017 15:31:27 -0500 Subject: [PATCH] Fix: use a free running channel key between sessiond and kernel consumer MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit We currently use the channel FD number opened by the session daemon to reference a channel in the consumer. This can lead to races where the session daemon destroys a channel and recreates one with the same FD number before the consumer has time to cleanup everything on its side, so all the commands in between that use that FD number has a key may end up working on the wrong objects. This fix introduces a free running counter as the channel key, so this decouples the channel key in the consumer from the channel FD in the session daemon. This fixes the race observed in stress tests. Signed-off-by: Julien Desfossez Signed-off-by: Jérémie Galarneau --- src/bin/lttng-sessiond/cmd.c | 4 ++-- src/bin/lttng-sessiond/kernel-consumer.c | 13 +++++++------ src/bin/lttng-sessiond/kernel.c | 16 +++++++++++++--- src/bin/lttng-sessiond/trace-kernel.c | 2 +- src/bin/lttng-sessiond/trace-kernel.h | 1 + 5 files changed, 24 insertions(+), 12 deletions(-) diff --git a/src/bin/lttng-sessiond/cmd.c b/src/bin/lttng-sessiond/cmd.c index 9d7425368..2c1ae3f51 100644 --- a/src/bin/lttng-sessiond/cmd.c +++ b/src/bin/lttng-sessiond/cmd.c @@ -167,14 +167,14 @@ static int get_kernel_runtime_stats(struct ltt_session *session, goto end; } - ret = consumer_get_discarded_events(session->id, kchan->fd, + ret = consumer_get_discarded_events(session->id, kchan->key, session->kernel_session->consumer, discarded_events); if (ret < 0) { goto end; } - ret = consumer_get_lost_packets(session->id, kchan->fd, + ret = consumer_get_lost_packets(session->id, kchan->key, session->kernel_session->consumer, lost_packets); if (ret < 0) { diff --git a/src/bin/lttng-sessiond/kernel-consumer.c b/src/bin/lttng-sessiond/kernel-consumer.c index bc481e5c4..89bf4596b 100644 --- a/src/bin/lttng-sessiond/kernel-consumer.c +++ b/src/bin/lttng-sessiond/kernel-consumer.c @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -129,7 +130,7 @@ int kernel_consumer_add_channel(struct consumer_socket *sock, /* Prep channel message structure */ consumer_init_channel_comm_msg(&lkm, LTTNG_CONSUMER_ADD_CHANNEL, - channel->fd, + channel->key, ksession->id, pathname, ksession->uid, @@ -160,7 +161,7 @@ int kernel_consumer_add_channel(struct consumer_socket *sock, status = notification_thread_command_add_channel( notification_thread_handle, session->name, ksession->uid, ksession->gid, - channel->channel->name, channel->fd, + channel->channel->name, channel->key, LTTNG_DOMAIN_KERNEL, channel->channel->attr.subbuf_size * channel->channel->attr.num_subbuf); rcu_read_unlock(); @@ -284,7 +285,7 @@ int kernel_consumer_add_stream(struct consumer_socket *sock, /* Prep stream consumer message */ consumer_init_stream_comm_msg(&lkm, LTTNG_CONSUMER_ADD_STREAM, - channel->fd, + channel->key, stream->fd, stream->cpu); @@ -438,7 +439,7 @@ int kernel_consumer_send_session(struct consumer_socket *sock, * Inform the relay that all the streams for the * channel were sent. */ - ret = kernel_consumer_streams_sent(sock, session, chan->fd); + ret = kernel_consumer_streams_sent(sock, session, chan->key); if (ret < 0) { goto error; } @@ -463,11 +464,11 @@ int kernel_consumer_destroy_channel(struct consumer_socket *socket, assert(channel); assert(socket); - DBG("Sending kernel consumer destroy channel key %d", channel->fd); + DBG("Sending kernel consumer destroy channel key %" PRIu64, channel->key); memset(&msg, 0, sizeof(msg)); msg.cmd_type = LTTNG_CONSUMER_DESTROY_CHANNEL; - msg.u.destroy_channel.key = channel->fd; + msg.u.destroy_channel.key = channel->key; pthread_mutex_lock(socket->lock); health_code_update(); diff --git a/src/bin/lttng-sessiond/kernel.c b/src/bin/lttng-sessiond/kernel.c index 8892e76dc..535a5b8e8 100644 --- a/src/bin/lttng-sessiond/kernel.c +++ b/src/bin/lttng-sessiond/kernel.c @@ -34,6 +34,12 @@ #include "kern-modules.h" #include "utils.h" +/* + * Key used to reference a channel between the sessiond and the consumer. This + * is only read and updated with the session_list lock held. + */ +static uint64_t next_kernel_channel_key; + /* * Add context on a kernel channel. * @@ -169,8 +175,10 @@ int kernel_create_channel(struct ltt_kernel_session *session, cds_list_add(&lkc->list, &session->channel_list.head); session->channel_count++; lkc->session = session; + lkc->key = ++next_kernel_channel_key; - DBG("Kernel channel %s created (fd: %d)", lkc->channel->name, lkc->fd); + DBG("Kernel channel %s created (fd: %d, key: %" PRIu64 ")", + lkc->channel->name, lkc->fd, lkc->key); return 0; @@ -291,7 +299,8 @@ int kernel_disable_channel(struct ltt_kernel_channel *chan) } chan->enabled = 0; - DBG("Kernel channel %s disabled (fd: %d)", chan->channel->name, chan->fd); + DBG("Kernel channel %s disabled (fd: %d, key: %" PRIu64 ")", + chan->channel->name, chan->fd, chan->key); return 0; @@ -315,7 +324,8 @@ int kernel_enable_channel(struct ltt_kernel_channel *chan) } chan->enabled = 1; - DBG("Kernel channel %s enabled (fd: %d)", chan->channel->name, chan->fd); + DBG("Kernel channel %s enabled (fd: %d, key: %" PRIu64 ")", + chan->channel->name, chan->fd, chan->key); return 0; diff --git a/src/bin/lttng-sessiond/trace-kernel.c b/src/bin/lttng-sessiond/trace-kernel.c index 52b8cc7e0..ef5abbab4 100644 --- a/src/bin/lttng-sessiond/trace-kernel.c +++ b/src/bin/lttng-sessiond/trace-kernel.c @@ -551,7 +551,7 @@ void trace_kernel_destroy_channel(struct ltt_kernel_channel *channel) && channel->published_to_notification_thread) { status = notification_thread_command_remove_channel( notification_thread_handle, - channel->fd, LTTNG_DOMAIN_KERNEL); + channel->key, LTTNG_DOMAIN_KERNEL); assert(status == LTTNG_OK); } free(channel->channel->attr.extended.ptr); diff --git a/src/bin/lttng-sessiond/trace-kernel.h b/src/bin/lttng-sessiond/trace-kernel.h index 2eb599272..9c52cee09 100644 --- a/src/bin/lttng-sessiond/trace-kernel.h +++ b/src/bin/lttng-sessiond/trace-kernel.h @@ -63,6 +63,7 @@ struct ltt_kernel_event { /* Kernel channel */ struct ltt_kernel_channel { int fd; + uint64_t key; /* Key to reference this channel with the consumer. */ int enabled; unsigned int stream_count; unsigned int event_count; -- 2.34.1