On Wed, Aug 12, 2026 at 03:46:58PM +0000, [email protected] 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 <[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?
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/[email protected]?part=2
