Re: [PATCH v4 6/9] ring-buffer: Dynamically calculate max_data_size

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Concurrent execution of ring_buffer_alloc_read_page() and ring_buffer_subbuf_order_set() leads to buddy allocator corruption and potential OOB memory access due to a mismatched page order when taking cpu_buffer->free_page.
--

--- Patch [6]: [PATCH v4 6/9] ring-buffer: Dynamically calculate max_data_size ---
commit a7794ac711440e6d89d8e099e4a88707ad083ed7
Author: Vincent Donnefort <vdonnefort@google.com>

ring-buffer: Dynamically calculate max_data_size

The ring buffer order can be dynamically modified and temporarily
disables writing to do so. It is therefore safe to use the updated value
to calculate the maximum event size which can be written onto the ring
buffer.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index f62d6853ee5c0..64bf4ac853f57 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -598,7 +598,6 @@ struct trace_buffer {
>  	struct ring_buffer_meta		*meta;
>  
>  	unsigned int			subbuf_order;
> -	unsigned int			max_data_size;
>  };
>  
>  static __always_inline unsigned int rb_subbuf_size(struct trace_buffer *buffer)

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but can concurrent execution of
ring_buffer_alloc_read_page() and ring_buffer_subbuf_order_set() lead to buddy
allocator corruption and potential out-of-bounds memory access?

When a user reads /sys/kernel/tracing/per_cpu/cpuX/trace_pipe_raw, it calls
ring_buffer_alloc_read_page(). If another thread concurrently resizes the ring
buffer via /sys/kernel/tracing/buffer_subbuf_order, it calls
ring_buffer_subbuf_order_set().

In ring_buffer_subbuf_order_set(), buffer->subbuf_order is updated locklessly
with respect to the reader:

kernel/trace/ring_buffer.c:ring_buffer_subbuf_order_set() {
    ...
	buffer->subbuf_order = order;

	/* Make sure all new buffers are allocated, before deleting the old ones */
	for_each_buffer_cpu(buffer, cpu) {
    ...
}

Before ring_buffer_subbuf_order_set() reaches its second loop to clear
cpu_buffer->free_page, ring_buffer_alloc_read_page() can fetch the new
order, grab the old page from cpu_buffer->free_page, and assign the new
order to bpage->order:

kernel/trace/ring_buffer.c:ring_buffer_alloc_read_page() {
    ...
	if (cpu_buffer->free_page) {
		bpage->data = cpu_buffer->free_page;
		cpu_buffer->free_page = NULL;
	}
    ...
}

When this page is later freed or swapped into the ring buffer, could this
cause memory corruption because writers believe the subbuffer size is the
new order, leading them to overwrite past the old order page?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812153311.2328812-1-vdonnefort@google.com?part=6