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

[Vincent Donnefort <vdonnefort@google.com>]

On Thu, Aug 13, 2026 at 09:31:53PM -0400, Steven Rostedt wrote:
> 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)
> 
>

ack.

> 
> 
> > 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.

Sounds good.

-- 
Vincent

> 
> 
> > @@ -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);