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 <mathieu.desnoyers@efficios.com>
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>

diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c
index 89dad5917884fd986376107d8189efe8039fbe85..e379171ae3df53ff46dbfb5ec6347aac687e396c 100644 (file)
--- 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 acefacb218e3b8316011dff9f7c289bcda9428bd..37adecbfe689e28a79303c601cefbe2ea6311677 100644 (file)
--- 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;