Re: [PATCH v4 5/9] tracing: Fix subbuf resize races in trace_pipe_raw readers

[Vincent Donnefort <vdonnefort@google.com>]

On Wed, Aug 12, 2026 at 03:47:05PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] A logic error in the retry loop of `tracing_buffers_read()` bypasses `ring_buffer_read_page()` on retry, leaking uninitialized or stale kernel memory to userspace.
> 
> Pre-existing issues:
> - [Critical] A race condition during subbuffer resize causes `cpu_buffer->free_page` to retain an old allocation size while `buffer->subbuf_order` reflects the new order, leading to a kernel buffer overflow.
> --
> 
> --- Patch [5]: [PATCH v4 5/9] tracing: Fix subbuf resize races in trace_pipe_raw readers ---
> commit c5c1a16fabb91c8f580283f14e5e76cc1ccb99b0
> Author: Vincent Donnefort <vdonnefort@google.com>
> 
> tracing: Fix subbuf resize races in trace_pipe_raw readers
> 
> Concurrent subbuffer resizes may crash trace_pipe_raw readers or leak
> uninitialized memory to userspace due to stale size values.
> 
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 94552a433228d..f62d6853ee5c0 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> 
> [ ... ]
> 
> > @@ -6988,22 +6988,34 @@ EXPORT_SYMBOL_GPL(ring_buffer_swap_cpu);
> >   * 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)
> >  {
> > +	struct buffer_data_read_page *bpage = prev;
> >  	struct ring_buffer_per_cpu *cpu_buffer;
> > -	struct buffer_data_read_page *bpage = NULL;
> >  	unsigned long flags;
> > +	unsigned int order;
> >  
> >  	if (!cpumask_test_cpu(cpu, buffer->cpumask))
> >  		return ERR_PTR(-ENODEV);
> >  
> > -	bpage = kzalloc_obj(*bpage);
> > -	if (!bpage)
> > -		return ERR_PTR(-ENOMEM);
> > -
> > -	bpage->order = buffer->subbuf_order;
> > +	order = buffer->subbuf_order;
> >  	cpu_buffer = buffer->buffers[cpu];
> > +
> > +	if (!bpage) {
> > +		bpage = kzalloc_obj(*bpage);
> > +		if (!bpage)
> > +			return ERR_PTR(-ENOMEM);
> > +	} else {
> > +		if (bpage->order == order)
> > +			return bpage;
> > +
> > +		free_pages((unsigned long)bpage->data, bpage->order);
> > +		bpage->data = NULL;
> > +	}
> > +
> > +	bpage->order = order;
> > +
> >  	local_irq_save(flags);
> >  	arch_spin_lock(&cpu_buffer->lock);
> >  
> >  	if (cpu_buffer->free_page) {
> >  		bpage->data = cpu_buffer->free_page;
> >  		cpu_buffer->free_page = NULL;
> >  	}
> 
> [Severity: Critical]
> This is a pre-existing issue, but can this lead to an out-of-bounds write
> during a subbuffer resize?
> 
> In ring_buffer_alloc_read_page(), the newly read buffer->subbuf_order is
> assigned to a page stolen from cpu_buffer->free_page without checking if
> their size orders match.
> 
> If a concurrent call to ring_buffer_subbuf_order_set() updates
> buffer->subbuf_order locklessly, and this function is called before
> cpu_buffer->free_page is cleared, it will steal the old free_page but assign
> it the new size order.
> 
> Will this malformed page be swapped into the ring buffer, allowing writers
> to write past its actual bounds?
> 
> [ ... ]
> 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 395238b2b715c..0409d20a168bb 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,31 @@ 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;
> >  	}
> 
> [Severity: High]
> Can the retry path bypass the actual read function and copy stale data
> to userspace?
> 
> When reading from an empty trace_pipe_raw file, ring_buffer_read_page()
> returns < 0. The code then blocks in wait_on_pipe(). Once woken up, it
> executes a goto again.
> 
> At the again label, info->read is 0 and info->spare is already allocated,
> so the condition info->read < spare_size will evaluate to true.
> 
> Does this cause execution to jump directly to the read label, bypassing
> ring_buffer_read_page(), and copy the uninitialized or stale info->spare
> page to userspace?

Arg, this one is real!

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260812153311.2328812-1-vdonnefort@google.com?part=5

-- 
Vincent