[PATCH] ring-buffer: serialize read-page order with subbuffer resize

[PATCH] ring-buffer: serialize read-page order with subbuffer resize

[Yousef Alhouseen]

ring_buffer_read_page() checks that its spare page has the current
subbuffer order before taking cpu_buffer->reader_lock. A concurrent
ring_buffer_subbuf_order_set() can change the order and replace the
reader page after that check. The reader then copies a larger subbuffer
into the old allocation, causing an out-of-bounds write.

Keep spare-page allocation and release under buffer->mutex, which already
serializes order changes. Move the read-side order check under
reader_lock, the lock used by resize when replacing per-CPU pages.

Fixes: f9b94daa542a ("ring-buffer: Set new size of the ring buffer sub page")
Reported-by: syzbot+2dd9d02f60775ce5c1fb@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2dd9d02f60775ce5c1fb
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 kernel/trace/ring_buffer.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 56a328e94395..eed5d7cffdee 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -6950,6 +6950,8 @@ ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu)
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
 		return ERR_PTR(-ENODEV);
 
+	guard(mutex)(&buffer->mutex);
+
 	bpage = kzalloc_obj(*bpage);
 	if (!bpage)
 		return ERR_PTR(-ENOMEM);
@@ -7000,6 +7002,8 @@ void ring_buffer_free_read_page(struct trace_buffer *buffer, int cpu,
 	if (!buffer || !buffer->buffers || !buffer->buffers[cpu])
 		return;
 
+	guard(mutex)(&buffer->mutex);
+
 	cpu_buffer = buffer->buffers[cpu];
 
 	/*
@@ -7091,14 +7095,13 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
 	if (!data_page || !data_page->data)
 		return -1;
 
-	if (data_page->order != buffer->subbuf_order)
-		return -1;
-
 	dpage = data_page->data;
 	if (!dpage)
 		return -1;
 
 	guard(raw_spinlock_irqsave)(&cpu_buffer->reader_lock);
+	if (data_page->order != buffer->subbuf_order)
+		return -1;
 
 	reader = rb_get_reader_page(cpu_buffer);
 	if (!reader)
-- 
2.54.0

Re: [PATCH] ring-buffer: serialize read-page order with subbuffer resize

[Steven Rostedt]

On Sun, 28 Jun 2026 02:46:53 +0200
Yousef Alhouseen <alhouseenyousef@gmail.com> wrote:

> ring_buffer_read_page() checks that its spare page has the current
> subbuffer order before taking cpu_buffer->reader_lock. A concurrent
> ring_buffer_subbuf_order_set() can change the order and replace the
> reader page after that check. The reader then copies a larger subbuffer
> into the old allocation, causing an out-of-bounds write.
> 
> Keep spare-page allocation and release under buffer->mutex, which already
> serializes order changes. Move the read-side order check under
> reader_lock, the lock used by resize when replacing per-CPU pages.
> 
> Fixes: f9b94daa542a ("ring-buffer: Set new size of the ring buffer sub page")
> Reported-by: syzbot+2dd9d02f60775ce5c1fb@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=2dd9d02f60775ce5c1fb
> Cc: stable@vger.kernel.org
> Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
> ---
>  kernel/trace/ring_buffer.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 56a328e94395..eed5d7cffdee 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -6950,6 +6950,8 @@ ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu)
>  	if (!cpumask_test_cpu(cpu, buffer->cpumask))
>  		return ERR_PTR(-ENODEV);
>  
> +	guard(mutex)(&buffer->mutex);
> +
>  	bpage = kzalloc_obj(*bpage);

First, do not grab locks around allocations unless the are really needed.
This is bad practice, as it extends the critical section and may even add
the allocation locking to the lock chain.

That said, just moving things around the current locks should work.

Like this (not compiled nor tested):

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 56a328e94395..8352f935a223 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -6954,11 +6954,11 @@ ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu)
 	if (!bpage)
 		return ERR_PTR(-ENOMEM);
 
-	bpage->order = buffer->subbuf_order;
 	cpu_buffer = buffer->buffers[cpu];
 	local_irq_save(flags);
 	arch_spin_lock(&cpu_buffer->lock);
 
+	bpage->order = buffer->subbuf_order;
 	if (cpu_buffer->free_page) {
 		bpage->data = cpu_buffer->free_page;
 		cpu_buffer->free_page = NULL;
@@ -7007,13 +7007,13 @@ void ring_buffer_free_read_page(struct trace_buffer *buffer, int cpu,
 	 * is different from the subbuffer order of the buffer -
 	 * we can't reuse it
 	 */
-	if (page_ref_count(page) > 1 || data_page->order != buffer->subbuf_order)
+	if (page_ref_count(page) > 1)
 		goto out;
 
 	local_irq_save(flags);
 	arch_spin_lock(&cpu_buffer->lock);
 
-	if (!cpu_buffer->free_page) {
+	if (!cpu_buffer->free_page && data_page->order == buffer->subbuf_order)
 		cpu_buffer->free_page = dpage;
 		dpage = NULL;
 	}
@@ -7091,15 +7091,15 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
 	if (!data_page || !data_page->data)
 		return -1;
 
-	if (data_page->order != buffer->subbuf_order)
-		return -1;
-
 	dpage = data_page->data;
 	if (!dpage)
 		return -1;
 
 	guard(raw_spinlock_irqsave)(&cpu_buffer->reader_lock);
 
+	if (data_page->order != buffer->subbuf_order)
+		return -1;
+
 	reader = rb_get_reader_page(cpu_buffer);
 	if (!reader)
 		return -1;

-- Steve

Re: [PATCH] ring-buffer: serialize read-page order with subbuffer resize

[Yousef Alhouseen]

Thanks, Steve. Moving the order reads under the existing per-CPU locks
is much tighter and avoids extending the allocation critical path.
I’ll compile and test this direction and send v2.

On Tue, 30 Jun 2026 10:14:25 -0400, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Sun, 28 Jun 2026 02:46:53 +0200
> Yousef Alhouseen <alhouseenyousef@gmail.com> wrote:
>
> > ring_buffer_read_page() checks that its spare page has the current
> > subbuffer order before taking cpu_buffer->reader_lock. A concurrent
> > ring_buffer_subbuf_order_set() can change the order and replace the
> > reader page after that check. The reader then copies a larger subbuffer
> > into the old allocation, causing an out-of-bounds write.
> >
> > Keep spare-page allocation and release under buffer->mutex, which already
> > serializes order changes. Move the read-side order check under
> > reader_lock, the lock used by resize when replacing per-CPU pages.
> >
> > Fixes: f9b94daa542a ("ring-buffer: Set new size of the ring buffer sub page")
> > Reported-by: syzbot+2dd9d02f60775ce5c1fb@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=2dd9d02f60775ce5c1fb
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
> > ---
> > kernel/trace/ring_buffer.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 56a328e94395..eed5d7cffdee 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -6950,6 +6950,8 @@ ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu)
> > if (!cpumask_test_cpu(cpu, buffer->cpumask))
> > return ERR_PTR(-ENODEV);
> >
> > + guard(mutex)(&buffer->mutex);
> > +
> > bpage = kzalloc_obj(*bpage);
>
> First, do not grab locks around allocations unless the are really needed.
> This is bad practice, as it extends the critical section and may even add
> the allocation locking to the lock chain.
>
> That said, just moving things around the current locks should work.
>
> Like this (not compiled nor tested):
>
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 56a328e94395..8352f935a223 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -6954,11 +6954,11 @@ ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu)
> if (!bpage)
> return ERR_PTR(-ENOMEM);
>
> - bpage->order = buffer->subbuf_order;
> cpu_buffer = buffer->buffers[cpu];
> local_irq_save(flags);
> arch_spin_lock(&cpu_buffer->lock);
>
> + bpage->order = buffer->subbuf_order;
> if (cpu_buffer->free_page) {
> bpage->data = cpu_buffer->free_page;
> cpu_buffer->free_page = NULL;
> @@ -7007,13 +7007,13 @@ void ring_buffer_free_read_page(struct trace_buffer *buffer, int cpu,
> * is different from the subbuffer order of the buffer -
> * we can't reuse it
> */
> - if (page_ref_count(page) > 1 || data_page->order != buffer->subbuf_order)
> + if (page_ref_count(page) > 1)
> goto out;
>
> local_irq_save(flags);
> arch_spin_lock(&cpu_buffer->lock);
>
> - if (!cpu_buffer->free_page) {
> + if (!cpu_buffer->free_page && data_page->order == buffer->subbuf_order)
> cpu_buffer->free_page = dpage;
> dpage = NULL;
> }
> @@ -7091,15 +7091,15 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
> if (!data_page || !data_page->data)
> return -1;
>
> - if (data_page->order != buffer->subbuf_order)
> - return -1;
> -
> dpage = data_page->data;
> if (!dpage)
> return -1;
>
> guard(raw_spinlock_irqsave)(&cpu_buffer->reader_lock);
>
> + if (data_page->order != buffer->subbuf_order)
> + return -1;
> +
> reader = rb_get_reader_page(cpu_buffer);
> if (!reader)
> return -1;
>
> -- Steve

Re: [PATCH] ring-buffer: serialize read-page order with subbuffer resize

[Yousef Alhouseen]

One issue turned up while checking the suggested locking:
ring_buffer_subbuf_order_set() writes buffer->subbuf_order before
taking reader_lock and never takes cpu_buffer->lock. An allocator can
therefore take cpu_buffer->lock after the new order is published but
before resize clears the old-order free_page, tag that old page with
the new order, and return it.

I can keep the allocations outside buffer->mutex and hold the mutex
only while snapshotting subbuf_order and taking or returning
free_page. That removes allocation from the critical section and
serializes the order/free-page pair with resize. Would you prefer
that, or should free_page and the order transition be synchronized
another way?

On Tue, 30 Jun 2026 10:14:25 -0400, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Sun, 28 Jun 2026 02:46:53 +0200
> Yousef Alhouseen <alhouseenyousef@gmail.com> wrote:
>
> > ring_buffer_read_page() checks that its spare page has the current
> > subbuffer order before taking cpu_buffer->reader_lock. A concurrent
> > ring_buffer_subbuf_order_set() can change the order and replace the
> > reader page after that check. The reader then copies a larger subbuffer
> > into the old allocation, causing an out-of-bounds write.
> >
> > Keep spare-page allocation and release under buffer->mutex, which already
> > serializes order changes. Move the read-side order check under
> > reader_lock, the lock used by resize when replacing per-CPU pages.
> >
> > Fixes: f9b94daa542a ("ring-buffer: Set new size of the ring buffer sub page")
> > Reported-by: syzbot+2dd9d02f60775ce5c1fb@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=2dd9d02f60775ce5c1fb
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
> > ---
> > kernel/trace/ring_buffer.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 56a328e94395..eed5d7cffdee 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -6950,6 +6950,8 @@ ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu)
> > if (!cpumask_test_cpu(cpu, buffer->cpumask))
> > return ERR_PTR(-ENODEV);
> >
> > + guard(mutex)(&buffer->mutex);
> > +
> > bpage = kzalloc_obj(*bpage);
>
> First, do not grab locks around allocations unless the are really needed.
> This is bad practice, as it extends the critical section and may even add
> the allocation locking to the lock chain.
>
> That said, just moving things around the current locks should work.
>
> Like this (not compiled nor tested):
>
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 56a328e94395..8352f935a223 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -6954,11 +6954,11 @@ ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu)
> if (!bpage)
> return ERR_PTR(-ENOMEM);
>
> - bpage->order = buffer->subbuf_order;
> cpu_buffer = buffer->buffers[cpu];
> local_irq_save(flags);
> arch_spin_lock(&cpu_buffer->lock);
>
> + bpage->order = buffer->subbuf_order;
> if (cpu_buffer->free_page) {
> bpage->data = cpu_buffer->free_page;
> cpu_buffer->free_page = NULL;
> @@ -7007,13 +7007,13 @@ void ring_buffer_free_read_page(struct trace_buffer *buffer, int cpu,
> * is different from the subbuffer order of the buffer -
> * we can't reuse it
> */
> - if (page_ref_count(page) > 1 || data_page->order != buffer->subbuf_order)
> + if (page_ref_count(page) > 1)
> goto out;
>
> local_irq_save(flags);
> arch_spin_lock(&cpu_buffer->lock);
>
> - if (!cpu_buffer->free_page) {
> + if (!cpu_buffer->free_page && data_page->order == buffer->subbuf_order)
> cpu_buffer->free_page = dpage;
> dpage = NULL;
> }
> @@ -7091,15 +7091,15 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
> if (!data_page || !data_page->data)
> return -1;
>
> - if (data_page->order != buffer->subbuf_order)
> - return -1;
> -
> dpage = data_page->data;
> if (!dpage)
> return -1;
>
> guard(raw_spinlock_irqsave)(&cpu_buffer->reader_lock);
>
> + if (data_page->order != buffer->subbuf_order)
> + return -1;
> +
> reader = rb_get_reader_page(cpu_buffer);
> if (!reader)
> return -1;
>
> -- Steve

Re: [PATCH] ring-buffer: serialize read-page order with subbuffer resize

[Steven Rostedt]

On Tue, 30 Jun 2026 13:45:05 -0700
Yousef Alhouseen <alhouseenyousef@gmail.com> wrote:

> One issue turned up while checking the suggested locking:
> ring_buffer_subbuf_order_set() writes buffer->subbuf_order before
> taking reader_lock and never takes cpu_buffer->lock. An allocator can
> therefore take cpu_buffer->lock after the new order is published but
> before resize clears the old-order free_page, tag that old page with
> the new order, and return it.
> 
> I can keep the allocations outside buffer->mutex and hold the mutex
> only while snapshotting subbuf_order and taking or returning
> free_page. That removes allocation from the critical section and
> serializes the order/free-page pair with resize. Would you prefer
> that, or should free_page and the order transition be synchronized
> another way?

Nothing should be reading when the subbuf_order is being updated. Let's add
a flag to state that it's being updated, and make all reads simply fail
during that time.

-- Steve

Re: [PATCH] ring-buffer: serialize read-page order with subbuffer resize

[Yousef Alhouseen]

Agreed. I’ll add an explicit resize-in-progress flag, set it around
the order transition, and make the external read-page
allocation/free/read paths reject work while it is set. I’ll check the
flag under the locks that serialize each path so it cannot race the
transition, then compile and test the resulting v2.

On Tue, 30 Jun 2026 17:16:03 -0400, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 30 Jun 2026 13:45:05 -0700
> Yousef Alhouseen <alhouseenyousef@gmail.com> wrote:
>
> > One issue turned up while checking the suggested locking:
> > ring_buffer_subbuf_order_set() writes buffer->subbuf_order before
> > taking reader_lock and never takes cpu_buffer->lock. An allocator can
> > therefore take cpu_buffer->lock after the new order is published but
> > before resize clears the old-order free_page, tag that old page with
> > the new order, and return it.
> >
> > I can keep the allocations outside buffer->mutex and hold the mutex
> > only while snapshotting subbuf_order and taking or returning
> > free_page. That removes allocation from the critical section and
> > serializes the order/free-page pair with resize. Would you prefer
> > that, or should free_page and the order transition be synchronized
> > another way?
>
> Nothing should be reading when the subbuf_order is being updated. Let's add
> a flag to state that it's being updated, and make all reads simply fail
> during that time.
>
> -- Steve