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
