Re: [PATCH v5 01/10] ring-buffer: Free cpu_buffer::free_page with subbuf_order

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

Pre-existing issues:
- [Critical] A data race and locking mismatch between `ring_buffer_subbuf_order_set()`, `ring_buffer_alloc_read_page()`, and `ring_buffer_free_read_page()` allows `cpu_buffer->free_page` to cache a page of an old order while `buffer->subbuf_order` reflects a new larger order. This leads to heap buffer overflow and buddy allocator corruption.
--

--- Patch [1]: [PATCH v5 01/10] ring-buffer: Free cpu_buffer::free_page with subbuf_order ---
commit f550c0bfeadcc22eca47316e1820b4d91f6ef261
Author: Vincent Donnefort <[email protected]>

ring-buffer: Free cpu_buffer::free_page with subbuf_order
    
When sub-buffers use an order greater than 0, cpu_buffer->free_page is
allocated with subbuf_order. Use the correct order for
cpu_buffer->free_page.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 2667992f0aa29..a3d28b2e2c94a 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -2631,7 +2631,7 @@ static void rb_free_cpu_buffer(struct ring_buffer_per_cpu *cpu_buffer)
>  		free_buffer_page(bpage);
>  	}
>  
> -	free_page((unsigned long)cpu_buffer->free_page);
> +	free_pages((unsigned long)cpu_buffer->free_page, cpu_buffer->buffer->subbuf_order);

[Severity: Critical]
This is a pre-existing issue, but does this change expose a buddy allocator
corruption if cpu_buffer->free_page contains a page of a smaller order than
buffer->subbuf_order?

A race condition between ring_buffer_subbuf_order_set() and 
ring_buffer_free_read_page() can cause a smaller order page to be stashed in
cpu_buffer->free_page while buffer->subbuf_order reflects a new larger
order.

If a trace reader calls ring_buffer_free_read_page(), it performs a lockless
check of buffer->subbuf_order:

kernel/trace/ring_buffer.c:ring_buffer_free_read_page() {
    ...
	if (page_ref_count(page) > 1 || data_page->order != buffer->subbuf_order)
		goto out;
    ...
}

If a concurrent write to buffer_subbuf_size_kb calls
ring_buffer_subbuf_order_set(), it updates buffer->subbuf_order without
holding cpu_buffer->lock, and clears cpu_buffer->free_page under
reader_lock.

The trace reader then acquires cpu_buffer->lock and stashes the old smaller
page into cpu_buffer->free_page, missing the clearance because of the lock
mismatch (reader_lock vs cpu_buffer->lock):

kernel/trace/ring_buffer.c:ring_buffer_free_read_page() {
    ...
	raw_spin_lock_irqsave(&cpu_buffer->lock, flags);
	if (!cpu_buffer->free_page) {
		cpu_buffer->free_page = dpage;
    ...
}

During teardown, free_pages() here would free the undersized page using the
new larger order, causing buddy allocator corruption. Additionally, if a reader
allocates this stashed page via ring_buffer_read_page(), it is swapped into
the active ring buffer where writers will overflow its bounds.

Can the locking be synchronized to prevent caching an old-order page?

>  
>  	kfree(cpu_buffer);
>  }

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