Re: [PATCH v4 2/9] ring-buffer: Hold cpu_buffer::lock when resizing a subbuf

[Vincent Donnefort <vdonnefort@google.com>]

On Wed, Aug 12, 2026 at 03:46:58PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] The patch attempts to fix a race with `cpu_buffer->free_page` by adding an `arch_spin_lock(&cpu_buffer->lock)`, but it leaves a severe Time-Of-Check to Time-Of-Use (TOCTOU) race unaddressed. `buffer->subbuf_order` and `buffer->subbuf_size` are updated globally outside of any per-CPU lock. If a reader (like `tracing_buffers_splice_read`) calls `ring_buffer_alloc_read_page()` concurrently, it locklessly reads the NEW `buffer->subbuf_order` but then retrieves the OLD, incorrectly-sized `free_page` from the locked cache. When `ring_buffer_read_page()` uses this page, it copies data up to the NEW `buffer->subbuf_size`, leading to a heap buffer overflow if the new order is larger. Similarly, `ring_buffer_free_read_page()` validates `data_page->order` locklessly; if the global order changes just after the check, it inserts a new-order page into `free_page`, which `ring_buffer_subbuf_order_set()` will later free using the `old_order`, corrupting the page allocator.
> - [High] The patch adds an unconditional `arch_spin_lock(&cpu_buffer->lock)` to `ring_buffer_subbuf_order_set()`. This creates an NMI deadlock window. If an NMI (e.g., from a watchdog or an Oops triggering `ftrace_dump_on_oops`) fires on the same CPU while this lock is held, the NMI handler may attempt to read the ring buffer. The read path (via `ring_buffer_peek()` -> `rb_buffer_peek()` -> `__rb_get_reader_page()`) unconditionally acquires the same `cpu_buffer->lock` using `arch_spin_lock()` without an NMI-safe trylock fallback. Because the interrupted context holds the lock, the NMI handler will spin forever, causing a hard lockup.
> --
> 
> --- Patch [2]: [PATCH v4 2/9] ring-buffer: Hold cpu_buffer::lock when resizing a subbuf ---
> commit 1db68f92346fd21f55d54529fb97b920f02a0136
> Author: Vincent Donnefort <vdonnefort@google.com>
> 
> ring-buffer: Hold cpu_buffer::lock when resizing a subbuf
> 
> This commit adds an arch_spin_lock() around the clearing of
> cpu_buffer->free_page in ring_buffer_subbuf_order_set(). This aims to
> prevent races with ring_buffer_alloc_read_page() and
> ring_buffer_free_read_page() during sub-buffer resizing.
> 
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index a3d28b2e2c94a..ec4f5a0c93e85 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -7431,8 +7431,10 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
> >  		cpu_buffer->nr_pages = cpu_buffer->nr_pages_to_update;
> >  		cpu_buffer->nr_pages_to_update = 0;
> >  
> > +		arch_spin_lock(&cpu_buffer->lock);
> 
> [Severity: Critical]
> This is a pre-existing issue, but is there a Time-Of-Check to Time-Of-Use
> (TOCTOU) race between the global order updates and this per-CPU lock?
> 
> Earlier in ring_buffer_subbuf_order_set(), buffer->subbuf_order and
> buffer->subbuf_size are updated globally outside of any per-CPU lock.
> 
> If a reader like tracing_buffers_splice_read() calls
> ring_buffer_alloc_read_page() concurrently, can it locklessly read the new
> buffer->subbuf_order, but then retrieve the old, incorrectly-sized
> free_page from the locked cache?
> 
> When ring_buffer_read_page() uses this page, wouldn't it copy data up to
> the new buffer->subbuf_size, leading to a heap buffer overflow if the new
> order is larger?
> 
> Similarly, ring_buffer_free_read_page() validates data_page->order against
> buffer->subbuf_order locklessly. If the global order changes right after
> the check, could it insert a new-order page into free_page, which
> ring_buffer_subbuf_order_set() will later free using the old order,
> corrupting the page allocator?

This is hopefully fixed in one of the following patch of this series.

> 
> [Severity: High]
> This is also a pre-existing issue, but does adding this unconditional
> arch_spin_lock() widen an NMI deadlock window?
> 
> If an NMI (such as a watchdog or an Oops triggering ftrace_dump_on_oops)
> fires on the same CPU while this lock is held, the NMI handler may attempt
> to read the ring buffer.
> 
> The read path via __rb_get_reader_page() unconditionally acquires the same
> cpu_buffer->lock using arch_spin_lock() without an NMI-safe trylock
> fallback.

This is under raw_spin_lock_irqsave(). I believe the panic path will first try
to issue normal IPIs.

-- 
Vincent

> 
> Because the interrupted context holds the lock, will the NMI handler spin
> forever, causing a hard lockup?
> 
> >  		old_free_data_page = cpu_buffer->free_page;
> >  		cpu_buffer->free_page = NULL;
> > +		arch_spin_unlock(&cpu_buffer->lock);
> >  
> >  		rb_head_page_activate(cpu_buffer);
> >
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260812153311.2328812-1-vdonnefort@google.com?part=2