From: Mathieu Desnoyers Date: Mon, 9 Jan 2017 16:23:16 +0000 (-0500) Subject: Fix: consumerd: order of metadata cache vs stream lock X-Git-Tag: v2.10.0-rc1~72 X-Git-Url: https://git.lttng.org/?p=lttng-tools.git;a=commitdiff_plain;h=3dad2c0ff117979532095b6bd83f59bf32a519d7 Fix: consumerd: order of metadata cache vs stream lock 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 --- 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;