Re: [PATCH v5 06/10] tracing: Fix subbuf resize races with trace_pipe_raw readers

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

Pre-existing issues:
- [Critical] Data Race / Double Free in tracing_buffers_read with concurrent readers
--

--- Patch [6]: [PATCH v5 06/10] tracing: Fix subbuf resize races with trace_pipe_raw readers ---
commit a4a4fc189c85fb142cc626dac47e73d02ae21aef
Author: Vincent Donnefort <[email protected]>

tracing: Fix subbuf resize races with trace_pipe_raw readers

Concurrent subbuffer resizes may crash trace_pipe_raw readers or leak
uninitialized memory to userspace due to stale size values. Modify
ring_buffer_alloc_read_page() to let it handle the resizing of
a previous buffer_data_read_page if necessary and add a new
ring_buffer_read_page_size() which enable ring-buffer users to not use
the racy ring_buffer_subbuf_size_get().

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index a00ab8a9cbd0c..83292d90599ec 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
[ ... ]
> +struct buffer_data_read_page *ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu,
> +							  struct buffer_data_read_page *prev)
>  {
[ ... ]
> +	if (prev && prev->order == order) {
> +		return prev;
> +	} else if (prev) {
> +		/* We can reuse prev, but we discard the payload */
> +		free_pages((unsigned long)prev->data, prev->order);
> +		prev->data = NULL;
> +		bpage = prev;
> +	} else {
[ ... ]
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 395238b2b715c..f9399f391ac6c 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
[ ... ]
> @@ -7080,8 +7080,8 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
[ ... ]
> +again:
> +	prev_spare = info->spare;
[ ... ]
> -	/* Do we have previous read data to read? */
> -	if (info->read < page_size)
> -		goto read;
> +	/* Make sure the read page order is aligned with the current buffer subbuf order */
> +	info->spare = ring_buffer_alloc_read_page(iter->array_buffer->buffer, iter->cpu_file,
> +						  prev_spare);

[Severity: Critical]
This isn't a bug introduced by this patch, but it looks like a pre-existing
issue with concurrent readers remains. If two threads share a trace_pipe_raw
file descriptor (e.g. via fork or pthreads) and call read() concurrently
while the subbuffer order is being changed, can they trigger a double-free?

Since vfs_read() does not hold f_pos_lock for this file (because
FMODE_ATOMIC_POS is cleared by nonseekable_open()), tracing_buffers_read()
can execute concurrently. If both threads read the same info->spare pointer
into prev_spare:

Thread 1:
    prev_spare = info->spare;

Thread 2:
    prev_spare = info->spare;

When both threads call into ring_buffer_alloc_read_page() with the same
prev pointer, and the subbuffer order has changed, wouldn't both threads
concurrently enter the 'else if (prev)' block and call free_pages() on the
same address without synchronization?

ring_buffer_alloc_read_page() {
    ...
    } else if (prev) {
        free_pages((unsigned long)prev->data, prev->order);
        prev->data = NULL;
    ...
}

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