[lttng-dev] [PATCH lttng-tools] Fix: consumerd: order of metadata cache vs stream lock

[Jérémie Galarneau]

Thanks for getting to the bottom of it! Merged in master, stable-2.8
and stable-2.9.

Jérémie

On 9 January 2017 at 11:23, Mathieu Desnoyers
<mathieu.desnoyers at efficios.com> wrote:
> 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 at efficios.com>
> ---
>  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 89dad59..e379171 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 acefacb..37adecb 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.1.4
>



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com