Re: [PATCH v7 1/3] tracing: Fix subbuf resize races with trace_pipe_raw readers

[sashiko-bot@kernel.org]

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

New issues:
- [High] Infinite busy-wait loop in tracing reads (tracing_buffers_read and tracing_buffers_splice_read) causing lockups during concurrent ring buffer resizes.

Pre-existing issues:
- [Critical] Concurrent io_uring reads trigger a double free on info->spare->data due to missing VFS locks.
- [High] Use-After-Free vulnerability in VFS pipe buffer destruction path due to lifecycle mismatch with the trace array.
--

--- Patch [1]: [PATCH v7 1/3] tracing: Fix subbuf resize races with trace_pipe_raw readers ---
commit 3b5685c224f6977e19f6580474cf69ff34b547a9
Author: Vincent Donnefort <vdonnefort@google.com>

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 handle resizing and add
ring_buffer_read_page_size() to avoid racy subbuf size gets.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 5fc009edc1ec9..ec13779922ffb 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
[ ... ]
> @@ -6990,56 +6990,78 @@ EXPORT_SYMBOL_GPL(ring_buffer_swap_cpu);
> +int ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu,
> +				struct buffer_data_read_page **rpage)
>  {
[ ... ]
> +	if (*rpage) {
> +		if ((*rpage)->order == order)
> +			return 0;
>  
> -	bpage->order = buffer->subbuf_order;
> +		/* We can reuse rpage, but we discard the payload */
> +		free_pages((unsigned long)(*rpage)->data, (*rpage)->order);
> +		(*rpage)->data = NULL;

[Severity: Critical]
This is a pre-existing issue, but concurrent io_uring reads can call vfs_read()
without holding f_pos_lock, leading to concurrent execution of
tracing_buffers_read().

Neither tracing_buffers_read() nor this function protects the shared
info->spare pointer with a lock.

If these threads race during a subbuffer resize, can both observe the order
mismatch simultaneously and execute free_pages() on the same data address,
causing a double free?

> +	} else {
> +		*rpage = kzalloc_obj(**rpage);
> +		if (!*rpage)
> +			return -ENOMEM;
> +	}
> +
> +	(*rpage)->order = order;
>  	cpu_buffer = buffer->buffers[cpu];
[ ... ]
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 395238b2b715c..737922b236d46 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
[ ... ]
> @@ -7128,7 +7116,9 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
>  				    iter->cpu_file, 0);
>  	trace_access_unlock(iter->cpu_file);
>  
> -	if (ret < 0) {
> +	if (ret == -EAGAIN) {
> +		goto again;
> +	} else if (ret < 0) {

[Severity: High]
If ring_buffer_read_page() returns -EAGAIN because the subbuffer order was
updated but cpu_buffer->reader_page->order hasn't been updated yet, this
branches back to the again label.

Since the loop does not yield the CPU or call cond_resched(), could this cause
a hard lockup if the reader and the resize thread are on the same CPU?

>  		if (trace_empty(iter) && !iter->closed) {
>  			if (update_last_data_if_empty(iter->tr))
>  				return 0;
[ ... ]
> @@ -7268,23 +7259,12 @@ ssize_t tracing_buffers_splice_read(struct file *file, loff_t *ppos,
>  	};
>  	struct buffer_ref *ref;
>  	bool woken = false;
> -	int page_size;
>  	int entries, i;
>  	ssize_t ret = 0;
[ ... ]
>  	for (i = 0; i < spd.nr_pages_max && len && entries; i++) {
> +		unsigned int page_size;
>  		struct page *page;
>  		int r;
>  
> @@ -7304,25 +7285,38 @@ ssize_t tracing_buffers_splice_read(struct file *file, loff_t *ppos,
>  
>  		refcount_set(&ref->refcount, 1);
>  		ref->buffer = iter->array_buffer->buffer;

[Severity: High]
This is a pre-existing issue, but splice_to_pipe() links kernel pages to a
pipe buffer and stashes ref->buffer as private data.

The VFS operation never acquires a reference on the trace_array. If the user
closes the trace file and deletes the trace instance, the trace_buffer is
freed.

When the pipe buffer is subsequently read or destroyed, won't
buffer_ref_release() dereference the dangling ref->buffer pointer when calling
ring_buffer_free_read_page(), causing a use-after-free?

> -		ref->page = ring_buffer_alloc_read_page(ref->buffer, iter->cpu_file);
> -		if (IS_ERR(ref->page)) {
> -			ret = PTR_ERR(ref->page);
> -			ref->page = NULL;
> +
> +new_read_page:
> +		ret = ring_buffer_alloc_read_page(ref->buffer, iter->cpu_file, &ref->rpage);
> +		if (ret) {
>  			kfree(ref);
>  			break;
>  		}
>  		ref->cpu = iter->cpu_file;
>  
> -		r = ring_buffer_read_page(ref->buffer, ref->page,
> -					  len, iter->cpu_file, 1);
> +		page_size = ring_buffer_read_page_size(ref->rpage);
> +
> +		r = -EINVAL;
> +		if (IS_ALIGNED(*ppos, page_size) && len >= page_size) {
> +			r = ring_buffer_read_page(ref->buffer, ref->rpage, len, iter->cpu_file, 1);
> +			if (r == -EAGAIN)
> +				goto new_read_page;

[Severity: High]
Similar to the read path above, this catches -EAGAIN and loops back to
new_read_page.

Because goto new_read_page executes continuously without releasing
trace_access_lock() or yielding the CPU, can this permanently deadlock the
system during a resize by preventing the resize thread from acquiring the
necessary locks?

> +		} else if (!i) {
> +			/*
> +			 * If the first iteration fails this is an invalid userspace input.
> +			 * Otherwise, this is because the subbuf order has been modified. Do not
> +			 * report an error and finish the read.
> +			 */
> +			ret = -EINVAL;
> +		}

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