From 3dad2c0ff117979532095b6bd83f59bf32a519d7 Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Mon, 9 Jan 2017 11:23:16 -0500 Subject: [PATCH] Fix: consumerd: order of metadata cache vs stream lock MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit The locking order comment in consumer.h is incorrect. First, its description of locking order is not in sync with the comment found in consumer-metadata-cache.h. The comment in struct consumer_metadata_cache only states that the metadata cache lock nests inside the consumer_data lock, and does not mention the stream lock, which implies that the metadata cache lock does NOT nest inside the stream lock. But let's investigate further to confirm: * lttng_consumer_read_subbuffer() acquires the stream lock, and then calls lttng_ustconsumer_read_subbuffer() with stream lock held, and then invokes commin_one_metadata_packet(), which acquires the metadata cache lock. * lttng_ustconsumer_sync_metadata() acquires the metadata stream lock, and calls commit_one_metadata_packet(), which takes the metadata cache lock. Therefore, update the comment in consumer.h to state that the metadata cache lock nests INSIDE the stream lock, and update consumer_del_metadata_stream() accordingly. This should take care of fixing the locking order reversal found by Coverity. CID 1368314 (#1 of 1): Thread deadlock (ORDER_REVERSAL) CID 1368319: Program hangs (ORDER_REVERSAL) Fixes: 5feafd4130 "Fix: protect the channel's metadata stream using the metadata cache lock" Fixes: 1ea6cc572b "Fix: lock nesting order reversed" Fixes: fb549e7ac2 "Fix: reverse channel and metadata cache lock nesting order" Reported-by: Coverity Scan Signed-off-by: Mathieu Desnoyers Signed-off-by: Jérémie Galarneau --- src/common/consumer/consumer.c | 4 ++-- src/common/consumer/consumer.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c index 89dad5917..e379171ae 100644 --- a/src/common/consumer/consumer.c +++ b/src/common/consumer/consumer.c @@ -2052,11 +2052,11 @@ void consumer_del_metadata_stream(struct lttng_consumer_stream *stream, pthread_mutex_lock(&consumer_data.lock); pthread_mutex_lock(&stream->chan->lock); + pthread_mutex_lock(&stream->lock); if (stream->chan->metadata_cache) { /* Only applicable to userspace consumers. */ pthread_mutex_lock(&stream->chan->metadata_cache->lock); } - pthread_mutex_lock(&stream->lock); /* Remove any reference to that stream. */ consumer_stream_delete(stream, ht); @@ -2080,10 +2080,10 @@ void consumer_del_metadata_stream(struct lttng_consumer_stream *stream, */ stream->chan->metadata_stream = NULL; - pthread_mutex_unlock(&stream->lock); if (stream->chan->metadata_cache) { pthread_mutex_unlock(&stream->chan->metadata_cache->lock); } + pthread_mutex_unlock(&stream->lock); pthread_mutex_unlock(&stream->chan->lock); pthread_mutex_unlock(&consumer_data.lock); diff --git a/src/common/consumer/consumer.h b/src/common/consumer/consumer.h index acefacb21..37adecbfe 100644 --- a/src/common/consumer/consumer.h +++ b/src/common/consumer/consumer.h @@ -337,9 +337,9 @@ struct lttng_consumer_stream { * Lock to use the stream FDs since they are used between threads. * * This is nested INSIDE the consumer_data lock. - * This is nested INSIDE the metadata cache lock. * This is nested INSIDE the channel lock. * This is nested INSIDE the channel timer lock. + * This is nested OUTSIDE the metadata cache lock. * This is nested OUTSIDE consumer_relayd_sock_pair lock. */ pthread_mutex_t lock; -- 2.34.1