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

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 <[email protected]>

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?

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

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/[email protected]?part=2