Re: [PATCH v5 06/10] tracing: Fix subbuf resize races with trace_pipe_raw readers

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 13 Aug 2026 14:11:48 +0100
Vincent Donnefort <vdonnefort@google.com> wrote:

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index a00ab8a9cbd0..83292d90599e 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -6976,34 +6976,52 @@ EXPORT_SYMBOL_GPL(ring_buffer_swap_cpu);
>   * ring_buffer_alloc_read_page - allocate a page to read from buffer
>   * @buffer: the buffer to allocate for.
>   * @cpu: the cpu buffer to allocate.
> + * @prev: The previous page to be repurposed (can be NULL).
>   *
> - * This function is used in conjunction with ring_buffer_read_page.
> + * This function is used in conjunction with ring_buffer_read_page().
>   * When reading a full page from the ring buffer, these functions
>   * can be used to speed up the process. The calling function should
>   * allocate a few pages first with this function. Then when it
>   * needs to get pages from the ring buffer, it passes the result
> - * of this function into ring_buffer_read_page, which will swap
> + * of this function into ring_buffer_read_page(), which will swap
>   * the page that was allocated, with the read page of the buffer.
>   *
> + * If @prev is provided, and it has a different order than the current
> + * subbuffer order, its payload will be freed and re-allocated. If it
> + * already matches the order, it is simply returned.
> + *
>   * Returns:
>   *  The page allocated, or ERR_PTR
>   */
> -struct buffer_data_read_page *
> -ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu)
> +struct buffer_data_read_page *ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu,
> +							  struct buffer_data_read_page *prev)

I think we should do this differently. I don't like the "prev" argument.
Instead, let's pass by address.

int ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu, struct buffer_data_read_page **rpage)




> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 395238b2b715..f9399f391ac6 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -7080,8 +7080,8 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
>  {
>  	struct ftrace_buffer_info *info = filp->private_data;
>  	struct trace_iterator *iter = &info->iter;
> -	void *trace_data;
> -	int page_size;
> +	void *trace_data, *prev_spare;
> +	unsigned int spare_size;
>  	ssize_t ret = 0;
>  	ssize_t size;
>  
> @@ -7091,36 +7091,30 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
>  	if (iter->snapshot && tracer_uses_snapshot(iter->tr->current_trace))
>  		return -EBUSY;
>  
> -	page_size = ring_buffer_subbuf_size_get(iter->array_buffer->buffer);
> +again:


> +	prev_spare = info->spare;
> +	if (prev_spare) {
> +		spare_size = ring_buffer_read_page_size(info->spare);
>  
> -	/* Make sure the spare matches the current sub buffer size */
> -	if (info->spare) {
> -		if (page_size != info->spare_size) {
> -			ring_buffer_free_read_page(iter->array_buffer->buffer,
> -						   info->spare_cpu, info->spare);
> -			info->spare = NULL;
> -		}
> +		/* Do we have previous read data to read? */
> +		if (info->read < spare_size)
> +			goto read;
>  	}
>  
> -	if (!info->spare) {
> -		info->spare = ring_buffer_alloc_read_page(iter->array_buffer->buffer,
> -							  iter->cpu_file);
> -		if (IS_ERR(info->spare)) {
> -			ret = PTR_ERR(info->spare);
> -			info->spare = NULL;
> -		} else {
> -			info->spare_cpu = iter->cpu_file;
> -			info->spare_size = page_size;
> -		}
> -	}
> -	if (!info->spare)
> +	/* Make sure the read page order is aligned with the current buffer subbuf order */
> +	info->spare = ring_buffer_alloc_read_page(iter->array_buffer->buffer, iter->cpu_file,
> +						  prev_spare);
> +	if (IS_ERR(info->spare)) {
> +		ret = PTR_ERR(info->spare);
> +		info->spare = NULL;
> +		ring_buffer_free_read_page(iter->array_buffer->buffer, info->spare_cpu, prev_spare);
>  		return ret;
> +	}


instead of the above:

	ret = ring_buffer_alloc_read_page(iter->array_buffer->buffer, iter->cpu_file,
					  &info->space);

Where the above could do (under lock):

	if (*rpage) {
		if (*rpage)->order == buffer->subbuf_order)
			return 0;
		ring_buffer_free_read_page(*rpage);
		*rpage = NULL;
	}

	*rpage = all the reader page;

That is, lets completely remove the responsibility of the user having to
keep track of the buffer order here.

>  
> -	/* Do we have previous read data to read? */
> -	if (info->read < page_size)
> -		goto read;
> +	spare_size = ring_buffer_read_page_size(info->spare);
> +	info->read = spare_size;
> +	info->spare_cpu = iter->cpu_file;
>  
> - again:
>  	trace_access_lock(iter->cpu_file);
>  	ret = ring_buffer_read_page(iter->array_buffer->buffer,
>  				    info->spare,

And if we could make ring_buffer_read_page() return -EAGAIN if the spare is
not the proper order. And only for that case.


> @@ -7129,6 +7123,10 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
>  	trace_access_unlock(iter->cpu_file);
>  
>  	if (ret < 0) {
> +		/* Did we race with ring_buffer_subbuf_order_set ? */
> +		if (spare_size != ring_buffer_subbuf_size_get(iter->array_buffer->buffer))
> +			goto again;

Then here we can just have:

		if (ret == -EAGAIN)
			goto again;

-- Steve


> +
>  		if (trace_empty(iter) && !iter->closed) {
>  			if (update_last_data_if_empty(iter->tr))
>  				return 0;
> @@ -7142,12 +7140,14 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
>  
>  			goto again;
>  		}
> +
>  		return 0;
>  	}
>  
>  	info->read = 0;
> +
>   read:
> -	size = page_size - info->read;
> +	size = spare_size - info->read;
>  	if (size > count)
>  		size = count;
>  	trace_data = ring_buffer_read_page_data(info->spare);