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

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

[Vincent Donnefort]

ring_buffer_subbuf_order_set() checks for resize_disabled twice under
the same buffer->mutex hold. Moreover, this check duplicates the logic
in ring_buffer_resize(). Create a common helper rb_resize_disabled() to
factor out this code.

Additionally, remove the unnecessary cpumask_test_cpu in
ring_buffer_subbuf_order_set().
for_each_buffer_cpu() already iterates over buffer->cpumask.

Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
---
 kernel/trace/ring_buffer.c | 67 ++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 39 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 04bb94c29f58..37801ac5e92e 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -3280,6 +3280,21 @@ static void update_pages_handler(struct work_struct *work)
 	complete(&cpu_buffer->update_done);
 }
 
+static bool rb_resize_disabled(struct trace_buffer *buffer, int cpu)
+{
+	lockdep_assert_held(&buffer->mutex);
+
+	if (cpu != RING_BUFFER_ALL_CPUS)
+		return atomic_read(&buffer->buffers[cpu]->resize_disabled);
+
+	for_each_buffer_cpu(buffer, cpu) {
+		if (atomic_read(&buffer->buffers[cpu]->resize_disabled))
+			return true;
+	}
+
+	return false;
+}
+
 /**
  * ring_buffer_resize - resize the ring buffer
  * @buffer: the buffer to resize.
@@ -3324,20 +3339,17 @@ int ring_buffer_resize(struct trace_buffer *buffer, unsigned long size,
 	if (nr_pages < 2)
 		nr_pages = 2;
 
-	if (cpu_id == RING_BUFFER_ALL_CPUS) {
-		/*
-		 * Don't succeed if resizing is disabled, as a reader might be
-		 * manipulating the ring buffer and is expecting a sane state while
-		 * this is true.
-		 */
-		for_each_buffer_cpu(buffer, cpu) {
-			cpu_buffer = buffer->buffers[cpu];
-			if (atomic_read(&cpu_buffer->resize_disabled)) {
-				err = -EBUSY;
-				goto out_err_unlock;
-			}
-		}
+	/*
+	 * Don't succeed if resizing is disabled, as a reader might be
+	 * manipulating the ring buffer and is expecting a sane state while
+	 * this is true.
+	 */
+	if (rb_resize_disabled(buffer, cpu_id)) {
+		err = -EBUSY;
+		goto out_err_unlock;
+	}
 
+	if (cpu_id == RING_BUFFER_ALL_CPUS) {
 		/* calculate the pages to update */
 		for_each_buffer_cpu(buffer, cpu) {
 			cpu_buffer = buffer->buffers[cpu];
@@ -3409,16 +3421,6 @@ int ring_buffer_resize(struct trace_buffer *buffer, unsigned long size,
 		if (nr_pages == cpu_buffer->nr_pages)
 			goto out;
 
-		/*
-		 * Don't succeed if resizing is disabled, as a reader might be
-		 * manipulating the ring buffer and is expecting a sane state while
-		 * this is true.
-		 */
-		if (atomic_read(&cpu_buffer->resize_disabled)) {
-			err = -EBUSY;
-			goto out_err_unlock;
-		}
-
 		cpu_buffer->nr_pages_to_update = nr_pages -
 						cpu_buffer->nr_pages;
 
@@ -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);
 
@@ -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;
-		}
-
 		/* 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));

base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
-- 
2.55.0.1032.g73a4cd73de-goog

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

[sashiko-bot]

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

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

[David CARLIER]

Hi Vincent.

> +   if (rb_resize_disabled(buffer, cpu_id)) {
> +           err = -EBUSY;
> +           goto out_err_unlock;
> +   }

For a single CPU, this now happens before the nr_pages == cpu_buffer->nr_pages
early exit, so writing the same size to per_cpu/cpuN/buffer_size_kb on a
mapped or persistent instance CPU fails with EBUSY instead of succeeding.
Intended ?

> Additionally, remove the unnecessary cpumask_test_cpu in
> ring_buffer_subbuf_order_set().

The install loop still has one.

Otherwise removing the second check is fine, the hotplug window is
covered by the cpus_read_lock() patch I sent separately.

Cheers.

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

[Vincent Donnefort]

On Mon, Sep 14, 2026 at 08:09:24PM +0100, David CARLIER wrote:
> Hi Vincent.
> 
> > +   if (rb_resize_disabled(buffer, cpu_id)) {
> > +           err = -EBUSY;
> > +           goto out_err_unlock;
> > +   }
> 
> For a single CPU, this now happens before the nr_pages == cpu_buffer->nr_pages
> early exit, so writing the same size to per_cpu/cpuN/buffer_size_kb on a
> mapped or persistent instance CPU fails with EBUSY instead of succeeding.
> Intended ?

Actually no, I didn't see that it is also "fixing" this discrepancy between the
per_cpu buffer_size_kb and the global one.

It seems to me better to align the behaviour for both interface, but then it is
touching something that is user interface... 

Steven, WDYS?

> 
> > Additionally, remove the unnecessary cpumask_test_cpu in
> > ring_buffer_subbuf_order_set().
> 
> The install loop still has one.
> 
> Otherwise removing the second check is fine, the hotplug window is
> covered by the cpus_read_lock() patch I sent separately.
> 
> Cheers.

-- 
Vincent

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

[Steven Rostedt]

On Tue, 15 Sep 2026 08:23:55 +0100
Vincent Donnefort <vdonnefort@google.com> wrote:

> > For a single CPU, this now happens before the nr_pages == cpu_buffer->nr_pages
> > early exit, so writing the same size to per_cpu/cpuN/buffer_size_kb on a
> > mapped or persistent instance CPU fails with EBUSY instead of succeeding.
> > Intended ?  
> 
> Actually no, I didn't see that it is also "fixing" this discrepancy between the
> per_cpu buffer_size_kb and the global one.
> 
> It seems to me better to align the behaviour for both interface, but then it is
> touching something that is user interface... 
> 
> Steven, WDYS?

I don't think we should worry about it. If something is mapped, then we
shouldn't be touching that file. Even writing the same value should
error out. There's no need to do that.

Hopefully it doesn't break anything because if it does, then yeah, we
will need to do something different here.

Oh, and can you break this up into two patches. One that adds this and
and one that does the:

   Additionally, remove the unnecessary cpumask_test_cpu in
   ring_buffer_subbuf_order_set(). for_each_buffer_cpu() already
   iterates over buffer->cpumask.

Hey, breaking up patches improves your commit count ;-)

-- Steve