Re: [PATCH v5 02/10] ring-buffer: Hold cpu_buffer::lock when resizing a subbuf

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Race condition during ring buffer resize leads to mismatched subbuffer order, causing heap overflow and buddy allocator corruption.
--

--- Patch [2]: [PATCH v5 02/10] ring-buffer: Hold cpu_buffer::lock when resizing a subbuf ---
commit b0f06308baef769af600659941453301acb7bea1
Author: Vincent Donnefort <[email protected]>

ring-buffer: Hold cpu_buffer::lock when resizing a subbuf

This commit adds locking around the clearing of cpu_buffer->free_page in
ring_buffer_subbuf_order_set() to prevent races with readers allocating
or freeing read pages.

> 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);
>  		old_free_data_page = cpu_buffer->free_page;
>  		cpu_buffer->free_page = NULL;
> +		arch_spin_unlock(&cpu_buffer->lock);

[Severity: Critical]
This is a pre-existing issue, but does adding this lock fully close the race
window with ring_buffer_alloc_read_page()?

Looking at ring_buffer_subbuf_order_set(), buffer->subbuf_order = order; is
set much earlier in the function, before allocating the new pages for each
CPU.

During this window, a concurrent reader calling ring_buffer_alloc_read_page()
can see the new global order but still claim the old, smaller free_page
from the per-CPU buffer:

ring_buffer_alloc_read_page() {
    bpage->order = buffer->subbuf_order; // gets new, larger order
    ...
    if (cpu_buffer->free_page) {
        bpage->data = cpu_buffer->free_page; // gets old, smaller page
        cpu_buffer->free_page = NULL;
    }
}

Later, if ring_buffer_free_read_page() is called, it will use the mismatched
bpage->order to free the old page, corrupting the buddy allocator.

Similarly, if ring_buffer_read_page() swaps this undersized page into the
ring buffer, producers will overflow the heap.

Should buffer->subbuf_order be updated after the per-CPU free pages are
safely cleared or replaced?

>  
>  		rb_head_page_activate(cpu_buffer);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2