Re: [PATCH v4 3/9] ring-buffer: Fix subbuf resize race with ring buffer readers

[Vincent Donnefort <vdonnefort@google.com>]

On Wed, Aug 12, 2026 at 03:53:12PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] Use-After-Free dereference of `reader_page` in `ring_buffer_read_start()` when bypassing buffer mutex.
> 
> Pre-existing issues:
> - [High] 32-bit integer overflow in ring buffer capacity calculation truncates large trace buffers.
> - [High] Out-of-bounds kernel memory write and VMAP flush in `ring_buffer_map_get_reader()` due to global capacity TOCTOU race.
> --
> 
> --- Patch [3]: [PATCH v4 3/9] ring-buffer: Fix subbuf resize race with ring buffer readers ---
> commit 9618f28d9a6939c645e7579fa798c42557eb075c
> Author: Vincent Donnefort <vdonnefort@google.com>
> 
> ring-buffer: Fix subbuf resize race with ring buffer readers
> 
> This commit changes the trace_buffer to use bpage order instead of
> trace_buffer::subbuf_size in ring_buffer_read_start() and
> ring_buffer_read_page(). It also removes trace_buffer::subbuf_size and
> introduces capacity accessors.
> 
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index ec4f5a0c93e85..97449423d3a68 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> [ ... ]
> > @@ -6380,36 +6417,44 @@ EXPORT_SYMBOL_GPL(ring_buffer_consume);
> >  struct ring_buffer_iter *
> >  ring_buffer_read_start(struct trace_buffer *buffer, int cpu, gfp_t flags)
> >  {
> > +	struct ring_buffer_iter *iter __free(kfree) = kzalloc_obj(*iter, flags);
> >  	struct ring_buffer_per_cpu *cpu_buffer;
> > -	struct ring_buffer_iter *iter;
> > -
> > -	if (!cpumask_test_cpu(cpu, buffer->cpumask))
> > -		return NULL;
> >  
> > -	iter = kzalloc_obj(*iter, flags);
> >  	if (!iter)
> >  		return NULL;
> >  
> > -	/* Holds the entire event: data and meta data */
> > -	iter->event_size = buffer->subbuf_size;
> > -	iter->event = kmalloc(iter->event_size, flags);
> > -	if (!iter->event) {
> > -		kfree(iter);
> > +	if (!cpumask_test_cpu(cpu, buffer->cpumask))
> >  		return NULL;
> > -	}
> >  
> >  	cpu_buffer = buffer->buffers[cpu];
> >  
> > -	iter->cpu_buffer = cpu_buffer;
> > +	/*
> > +	 * Only KDB is using GFP_ATOMIC, for the others, lock the buffer to
> > +	 * prevent concurrent resizing.
> > +	 */
> > +	if (gfpflags_allow_blocking(flags))
> > +		mutex_lock(&buffer->mutex);
> >  
> >  	atomic_inc(&cpu_buffer->resize_disabled);
> >  
> > +	if (gfpflags_allow_blocking(flags))
> > +		mutex_unlock(&buffer->mutex);
> > +
> > +	/* Holds the entire event: data and meta data. */
> > +	iter->event_size = rb_page_capacity(READ_ONCE(cpu_buffer->reader_page));
> 
> [Severity: High]
> Can this result in a use-after-free dereference of the reader page?
> 
> When ring_buffer_read_start() is called with GFP_ATOMIC, it skips acquiring
> buffer->mutex and only increments cpu_buffer->resize_disabled.
> 
> If a concurrent ring_buffer_subbuf_order_set() has already passed its
> resize_disabled check, it will proceed to allocate new pages and free the old
> reader page.
> 
> A thread executing ring_buffer_read_start() could load the old reader page
> pointer just before it is freed by the other CPU, dereferencing freed memory
> when accessing the order field.

Yep, as the comment says, we only bypass for KDB.

> 
> > +	iter->event = kmalloc(iter->event_size, flags);
> > +	if (!iter->event) {
> > +		atomic_dec(&cpu_buffer->resize_disabled);
> > +		return NULL;
> > +	}
> [ ... ]
> > @@ -7367,8 +7413,8 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
> >  		}
> >  
> >  		/* Update the number of pages to match the new size */
> > -		nr_pages = old_size * buffer->buffers[cpu]->nr_pages;
> > -		nr_pages = DIV_ROUND_UP(nr_pages, buffer->subbuf_size);
> > +		nr_pages = old_capacity * buffer->buffers[cpu]->nr_pages;
> 
> [Severity: High]
> This is a pre-existing issue, but can this 32-bit integer capacity calculation
> truncate large trace buffers?
> 
> For buffers larger than 4GB, the multiplication of old_capacity and
> buffer->buffers[cpu]->nr_pages can exceed 32 bits. Because nr_pages is a
> 32-bit signed integer, the result is truncated, and the buffer is shrunk to a
> tiny fraction of its intended size during a resize operation.

This is fixed in a later patch.

> 
> > +		nr_pages = DIV_ROUND_UP(nr_pages, rb_subbuf_capacity(buffer));
> >  
> >  		/* we need a minimum of two pages */
> >  		if (nr_pages < 2)
> [ ... ]
> > @@ -7896,7 +7941,7 @@ int ring_buffer_map_get_reader(struct trace_buffer *buffer, int cpu)
> >  			 * missed events, then record it there.
> >  			 */
> >  			commit = rb_page_size(reader);
> > -			if (buffer->subbuf_size - commit >= sizeof(missed_events)) {
> > +			if (rb_subbuf_capacity(buffer) - commit >= sizeof(missed_events)) {
> 
> [Severity: High]
> This is a pre-existing issue, but does this create a time-of-check to
> time-of-use race leading to an out-of-bounds kernel memory write and
> VMAP flush?

This is fixed in a later patch.

> 
> During a sub-buffer order resize, ring_buffer_subbuf_order_set() modifies
> buffer->subbuf_order globally before verifying if resize_disabled is set. If
> the buffer is currently mapped, the resize aborts and restores the old order.
> 
> In that race window, a concurrent ring_buffer_map_get_reader() call will see
> the new, larger capacity via rb_subbuf_capacity(buffer) here. It could then
> write missed_events past the end of the old, smaller reader page, and
> subsequently flush memory beyond the mapped VMAP range.
> 
> >  				memcpy(&dpage->data[commit], &missed_events,
> >  				       sizeof(missed_events));
> >  				local_add(RB_MISSED_STORED, &dpage->commit);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260812153311.2328812-1-vdonnefort@google.com?part=3

-- 
Vincent