Re: [PATCH] ring-buffer: Check resize_disabled before publishing the new subbuf order

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing cpus_read_lock() allows concurrent CPU hotplug events to crash ring_buffer_subbuf_order_set() via uninitialized list dereference or NULL pointer dereference.
--

commit d554518f86e022de0069fc76033b9d71e9046148
Author: David Carlier <devnexen@gmail.com>

ring-buffer: Check resize_disabled before publishing the new subbuf order

This commit checks if resizing is disabled on any CPU before applying
the new sub-buffer order. This aims to prevent a race condition with
mmap fast paths mapping already-mapped CPUs with the new order.

Cc: stable@vger.kernel.org

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 9c03a555a6ba2..d7e5e4620d096 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -7474,6 +7474,14 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
>  
>  	old_capacity = rb_subbuf_capacity(buffer);
>  
> +	/* The mmap fast path reads subbuf_order without buffer->mutex. */
> +	for_each_buffer_cpu(buffer, cpu) {
> +		if (!cpumask_test_cpu(cpu, buffer->cpumask))
> +			continue;
> +		if (atomic_read(&buffer->buffers[cpu]->resize_disabled))
> +			return -EBUSY;
> +	}
> +

[Severity: High]
Can this new loop race with concurrent CPU hotplug events and trigger a NULL
pointer dereference?

Unlike ring_buffer_resize(), ring_buffer_subbuf_order_set() doesn't appear
to take cpus_read_lock(). Meanwhile, trace_rb_cpu_prepare() can update
buffer->cpumask without holding buffer->mutex.

On weakly ordered architectures, could the lack of an smp_rmb() here cause
the reader to observe a newly added CPU in buffer->cpumask but load a NULL
buffer->buffers[cpu]?

Additionally, without cpus_read_lock(), if a CPU is added to the cpumask
between the allocation loop and the final page update loop later in this
function, new_pages will be an empty list head:

ring_buffer_subbuf_order_set() {
    ...
    /* One page was allocated for the reader page */
    cpu_buffer->reader_page = list_entry(cpu_buffer->new_pages.next,
                                         struct buffer_page, list);
    list_del_init(&cpu_buffer->reader_page->list);
    ...
}

Could this calculate a fake pointer overlapping the cpu_buffer struct and
corrupt memory when modified?

>  	atomic_inc(&buffer->record_disabled);
>  
>  	/* Make sure all commits have finished */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912103938.1127021-1-devnexen@gmail.com?part=1