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
