Re: [PATCH v5 03/10] ring-buffer: Make cpu_buffer::free_page a buffer_data_read_page

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] `ring_buffer_alloc_read_page()` returning a cached page with an obsolete order causes `tracing_buffers_read()` to permanently return EOF to userspace.
--

--- Patch [3]: [PATCH v5 03/10] ring-buffer: Make cpu_buffer::free_page a buffer_data_read_page ---
commit 19f06657067d8dbcf108f6bacc6885d031739166
Author: Vincent Donnefort <[email protected]>

    ring-buffer: Make cpu_buffer::free_page a buffer_data_read_page

This commit changes the free_page cache to store the page order alongside
the page address. This fixes a mismatch when a cached reader page is
discarded after a concurrent ring buffer resize changes the global
subbuf_order.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index ec4f5a0c93e85..b6fa258aafe2c 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
[ ... ]
> @@ -6962,9 +6962,9 @@ ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu)
>  	local_irq_save(flags);
>  	arch_spin_lock(&cpu_buffer->lock);
>  
> -	if (cpu_buffer->free_page) {
> -		bpage->data = cpu_buffer->free_page;
> -		cpu_buffer->free_page = NULL;
> +	if (cpu_buffer->free_page.data) {
> +		*bpage = cpu_buffer->free_page;
> +		cpu_buffer->free_page.data = NULL;

[Severity: High]
Does this struct assignment cause tracing_buffers_read() to permanently
return EOF to userspace during a concurrent resize?

When a resize happens in ring_buffer_subbuf_order_set(),
buffer->subbuf_order is updated first:

	buffer->subbuf_order = order;

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

This opens a large race window before cpu_buffer->free_page.data is
cleared for each CPU.

If a user reads from trace_pipe_raw during this window,
ring_buffer_alloc_read_page() allocates bpage and initially sets
bpage->order to the new buffer->subbuf_order. However, the struct
assignment *bpage = cpu_buffer->free_page copies the old free_page,
overwriting bpage->order with the old order.

Later in tracing_buffers_read(), the caller caches this mismatched page
and sets spare_size to the new size:

		} else {
			info->spare_cpu = iter->cpu_file;
			info->spare_size = page_size;
		}

Because the orders don't match, ring_buffer_read_page() rejects the page:

	if (data_page->order != buffer->subbuf_order)
		return -1;

This returns -1, which tracing_buffers_read() treats as EOF. Subsequent
reads see info->spare_size == page_size, reuse the bad page, and
permanently return EOF.

>  	}
>  
>  	arch_spin_unlock(&cpu_buffer->lock);

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