Re: [PATCH v5 06/10] tracing: Fix subbuf resize races with trace_pipe_raw readers
From: Steven Rostedt <rostedt@goodmis.org>
Date: 2026-08-14 01:31:41
Also in:
lkml
On Thu, 13 Aug 2026 14:11:48 +0100 Vincent Donnefort [off-list ref] wrote:
quoted hunk ↗ jump to hunk
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c index a00ab8a9cbd0..83292d90599e 100644 --- a/kernel/trace/ring_buffer.c +++ b/kernel/trace/ring_buffer.c@@ -6976,34 +6976,52 @@ EXPORT_SYMBOL_GPL(ring_buffer_swap_cpu); * ring_buffer_alloc_read_page - allocate a page to read from buffer * @buffer: the buffer to allocate for. * @cpu: the cpu buffer to allocate. + * @prev: The previous page to be repurposed (can be NULL). * - * This function is used in conjunction with ring_buffer_read_page. + * This function is used in conjunction with ring_buffer_read_page(). * When reading a full page from the ring buffer, these functions * can be used to speed up the process. The calling function should * allocate a few pages first with this function. Then when it * needs to get pages from the ring buffer, it passes the result - * of this function into ring_buffer_read_page, which will swap + * of this function into ring_buffer_read_page(), which will swap * the page that was allocated, with the read page of the buffer. * + * If @prev is provided, and it has a different order than the current + * subbuffer order, its payload will be freed and re-allocated. If it + * already matches the order, it is simply returned. + * * Returns: * The page allocated, or ERR_PTR */ -struct buffer_data_read_page * -ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu) +struct buffer_data_read_page *ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu, + struct buffer_data_read_page *prev)
I think we should do this differently. I don't like the "prev" argument. Instead, let's pass by address. int ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu, struct buffer_data_read_page **rpage)
quoted hunk ↗ jump to hunk
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 395238b2b715..f9399f391ac6 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, { struct ftrace_buffer_info *info = filp->private_data; struct trace_iterator *iter = &info->iter; - void *trace_data; - int page_size; + void *trace_data, *prev_spare; + unsigned int spare_size; ssize_t ret = 0; ssize_t size;@@ -7091,36 +7091,30 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf, if (iter->snapshot && tracer_uses_snapshot(iter->tr->current_trace)) return -EBUSY; - page_size = ring_buffer_subbuf_size_get(iter->array_buffer->buffer); +again:
+ prev_spare = info->spare;
+ if (prev_spare) {
+ spare_size = ring_buffer_read_page_size(info->spare);
- /* Make sure the spare matches the current sub buffer size */
- if (info->spare) {
- if (page_size != info->spare_size) {
- ring_buffer_free_read_page(iter->array_buffer->buffer,
- info->spare_cpu, info->spare);
- info->spare = NULL;
- }
+ /* Do we have previous read data to read? */
+ if (info->read < spare_size)
+ goto read;
}
- if (!info->spare) {
- info->spare = ring_buffer_alloc_read_page(iter->array_buffer->buffer,
- iter->cpu_file);
- if (IS_ERR(info->spare)) {
- ret = PTR_ERR(info->spare);
- info->spare = NULL;
- } else {
- info->spare_cpu = iter->cpu_file;
- info->spare_size = page_size;
- }
- }
- if (!info->spare)
+ /* 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);
+ if (IS_ERR(info->spare)) {
+ ret = PTR_ERR(info->spare);
+ info->spare = NULL;
+ ring_buffer_free_read_page(iter->array_buffer->buffer, info->spare_cpu, prev_spare);
return ret;
+ }
instead of the above:
ret = ring_buffer_alloc_read_page(iter->array_buffer->buffer, iter->cpu_file,
&info->space);
Where the above could do (under lock):
if (*rpage) {
if (*rpage)->order == buffer->subbuf_order)
return 0;
ring_buffer_free_read_page(*rpage);
*rpage = NULL;
}
*rpage = all the reader page;
That is, lets completely remove the responsibility of the user having to
keep track of the buffer order here.
- /* Do we have previous read data to read? */ - if (info->read < page_size) - goto read; + spare_size = ring_buffer_read_page_size(info->spare); + info->read = spare_size; + info->spare_cpu = iter->cpu_file; - again: trace_access_lock(iter->cpu_file); ret = ring_buffer_read_page(iter->array_buffer->buffer, info->spare,
And if we could make ring_buffer_read_page() return -EAGAIN if the spare is not the proper order. And only for that case.
quoted hunk ↗ jump to hunk
@@ -7129,6 +7123,10 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf, trace_access_unlock(iter->cpu_file); if (ret < 0) { + /* Did we race with ring_buffer_subbuf_order_set ? */ + if (spare_size != ring_buffer_subbuf_size_get(iter->array_buffer->buffer)) + goto again;
Then here we can just have: if (ret == -EAGAIN) goto again; -- Steve
quoted hunk ↗ jump to hunk
+ if (trace_empty(iter) && !iter->closed) { if (update_last_data_if_empty(iter->tr)) return 0;@@ -7142,12 +7140,14 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf, goto again; } + return 0; } info->read = 0; + read: - size = page_size - info->read; + size = spare_size - info->read; if (size > count) size = count; trace_data = ring_buffer_read_page_data(info->spare);