[lttng-dev] [PATCH lttng-tools] Fix: acquire stream lock during kernel metadata snapshot

[Jérémie Galarneau]

Merged in master, stable-2.11, stable-2.10, and stable-2.9.

Thanks,
Jérémie

On Mon, Sep 10, 2018 at 08:09:12PM -0400, Jonathan Rajotte wrote:
> From: Jérémie Galarneau <jeremie.galarneau at efficios.com>
> 
> The stream lock is not taken when interacting with the kernel
> metadata stream that is created at the time a snapshot is taken.
> 
> This was noticed while reviewing the code for an unrelated reason,
> so there is no known problem caused by this. Nevertheless, this
> is incorrect as the stream is globally visible in the consumer.
> 
> Moreover, the stream was not cleaned-up which can cause a leak
> whenever a metadata snapshot fails.
> 
> Signed-off-by: Jérémie Galarneau <jeremie.galarneau at efficios.com>
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien at efficios.com>
> ---
>  src/common/kernel-consumer/kernel-consumer.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/src/common/kernel-consumer/kernel-consumer.c b/src/common/kernel-consumer/kernel-consumer.c
> index 87ade12fa..3455f827b 100644
> --- a/src/common/kernel-consumer/kernel-consumer.c
> +++ b/src/common/kernel-consumer/kernel-consumer.c
> @@ -336,7 +336,7 @@ end:
>   *
>   * Returns 0 on success, < 0 on error
>   */
> -int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
> +static int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
>  		uint64_t relayd_id, struct lttng_consumer_local_data *ctx)
>  {
>  	int ret, use_relayd = 0;
> @@ -355,11 +355,12 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
>  	if (!metadata_channel) {
>  		ERR("Kernel snapshot metadata not found for key %" PRIu64, key);
>  		ret = -1;
> -		goto error;
> +		goto error_no_channel;
>  	}
>  
>  	metadata_stream = metadata_channel->metadata_stream;
>  	assert(metadata_stream);
> +	pthread_mutex_lock(&metadata_stream->lock);
>  
>  	/* Flag once that we have a valid relayd for the stream. */
>  	if (relayd_id != (uint64_t) -1ULL) {
> @@ -369,7 +370,7 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
>  	if (use_relayd) {
>  		ret = consumer_send_relayd_stream(metadata_stream, path);
>  		if (ret < 0) {
> -			goto error;
> +			goto error_snapshot;
>  		}
>  	} else {
>  		ret = utils_create_stream_file(path, metadata_stream->name,
> @@ -377,7 +378,7 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
>  				metadata_stream->tracefile_count_current,
>  				metadata_stream->uid, metadata_stream->gid, NULL);
>  		if (ret < 0) {
> -			goto error;
> +			goto error_snapshot;
>  		}
>  		metadata_stream->out_fd = ret;
>  	}
> @@ -390,7 +391,8 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
>  			if (ret_read != -EAGAIN) {
>  				ERR("Kernel snapshot reading metadata subbuffer (ret: %zd)",
>  						ret_read);
> -				goto error;
> +				ret = ret_read;
> +				goto error_snapshot;
>  			}
>  			/* ret_read is negative at this point so we will exit the loop. */
>  			continue;
> @@ -415,11 +417,12 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
>  	}
>  
>  	ret = 0;
> -
> +error_snapshot:
> +	pthread_mutex_unlock(&metadata_stream->lock);
>  	cds_list_del(&metadata_stream->send_node);
>  	consumer_stream_destroy(metadata_stream, NULL);
>  	metadata_channel->metadata_stream = NULL;
> -error:
> +error_no_channel:
>  	rcu_read_unlock();
>  	return ret;
>  }
> -- 
> 2.17.1
>