Re: [PATCH v1] ring-buffer: Clean up resize_disabled checks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing CPU hotplug lock in ring_buffer_subbuf_order_set() causes NULL/poison pointer dereference on concurrent hotplug.
--

commit 6a990a7dda41a1ef54e4e559d824ba3e1166dfe2
Author: Vincent Donnefort <vdonnefort@google.com>

ring-buffer: Clean up resize_disabled checks

This patch creates a common helper rb_resize_disabled() to refactor duplicate
checks in ring_buffer_subbuf_order_set() and ring_buffer_resize(). It also
removes an unnecessary cpumask_test_cpu() in ring_buffer_subbuf_order_set().

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 04bb94c29f587..37801ac5e92e2 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c

[ ... ]

> @@ -7473,13 +7475,9 @@ 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;
> -	}
> +	/* Check it is resizable before we touch subbuf_order */
> +	if (rb_resize_disabled(buffer, RING_BUFFER_ALL_CPUS))
> +		return -EBUSY;
>  
>  	atomic_inc(&buffer->record_disabled);

[Severity: High]
This is a pre-existing issue, but since we are modifying
ring_buffer_subbuf_order_set(), is there a missing CPU hotplug lock here
that could cause a crash?

Unlike ring_buffer_resize() which takes cpus_read_lock(), this function
takes buffer->mutex but fails to protect buffer->cpumask against
concurrent hotplug modifications.

>  
> @@ -7490,17 +7488,8 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
>  
>  	/* Make sure all new buffers are allocated, before deleting the old ones */
>  	for_each_buffer_cpu(buffer, cpu) {
> -
> -		if (!cpumask_test_cpu(cpu, buffer->cpumask))
> -			continue;
> -
>  		cpu_buffer = buffer->buffers[cpu];
>  
> -		if (atomic_read(&cpu_buffer->resize_disabled)) {
> -			err = -EBUSY;
> -			goto error;
> -		}
> -

[Severity: High]
This is a pre-existing issue related to the missing hotplug lock above.
In this first loop of ring_buffer_subbuf_order_set(), new pages are allocated
only for CPUs present in the mask at this point:

	for_each_buffer_cpu(buffer, cpu) {
		...
		/* Allocate the new size buffer */
		INIT_LIST_HEAD(&cpu_buffer->new_pages);
		if (__rb_allocate_pages(cpu_buffer, nr_pages,
					&cpu_buffer->new_pages)) {
			...
		}
	}

Later, the second loop iterates over the cpumask again:

	for_each_buffer_cpu(buffer, cpu) {
		...
		/* 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);

If the cpumask expands between the two loops due to concurrent hotplug,
the second loop will process the newly added CPU.

However, for that new CPU, the new_pages list was not populated by the
first loop. Instead, trace_rb_cpu_prepare() will have left it in a
LIST_POISON1 state (via list_del()), causing a deterministic kernel crash
when dereferencing new_pages.next.

Could cpus_read_lock() be added at the beginning of
ring_buffer_subbuf_order_set() to protect against this?

>  		/* Update the number of pages to match the new size */
>  		nr_pages = old_capacity * buffer->buffers[cpu]->nr_pages;
>  		nr_pages = DIV_ROUND_UP(nr_pages, rb_subbuf_capacity(buffer));

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