[PATCH] tracing: ring_buffer: wait for writers before removing pages

[PATCH] tracing: ring_buffer: wait for writers before removing pages

[Haoli Zhang]

A writer can still be using the ring buffer page list when a concurrent
resize removes and frees pages. Disabling recording prevents new writers
from entering, but it does not wait for writers that are already in the
reserve/commit path. The reader lock protects reader-side page operations
and does not provide that writer-side synchronization.

Disable recording before waiting for a grace period, then wait for
pre-existing writers to finish before taking the reader lock and changing
the page list. Keep recording disabled until after the page list has been
updated and the reader lock has been released. The removed pages are
unlinked and subsequently freed.

This adds a grace-period wait to the resize path; it does not add
synchronization to the normal ring-buffer write path.

Reported-by: syzbot+37e1793bdb0061ea3a47@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=37e1793bdb0061ea3a47
Co-developed-by: Jinning Zhang <25031212117@stu.xidian.edu.cn>
Signed-off-by: Jinning Zhang <25031212117@stu.xidian.edu.cn>
Co-developed-by: Bin Yu <byu@xidian.edu.cn>
Signed-off-by: Bin Yu <byu@xidian.edu.cn>
Signed-off-by: Haoli Zhang <26031212468@stu.xidian.edu.cn>
---
 kernel/trace/ring_buffer.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 04bb94c..d92e001 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -3078,8 +3078,11 @@ rb_remove_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned long nr_pages)
 
 	head_bit = 0;
 
-	raw_spin_lock_irq(&cpu_buffer->reader_lock);
 	atomic_inc(&cpu_buffer->record_disabled);
+	/* Wait for in-flight writers to finish before unlinking pages. */
+	synchronize_rcu();
+
+	raw_spin_lock_irq(&cpu_buffer->reader_lock);
 	/*
 	 * We don't race with the readers since we have acquired the reader
 	 * lock. We also don't race with writers after disabling recording.
@@ -3132,8 +3135,8 @@ rb_remove_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned long nr_pages)
 						struct buffer_page, list);
 
 	/* pages are removed, resume tracing and then free the pages */
-	atomic_dec(&cpu_buffer->record_disabled);
 	raw_spin_unlock_irq(&cpu_buffer->reader_lock);
+	atomic_dec(&cpu_buffer->record_disabled);
 
 	RB_WARN_ON(cpu_buffer, list_empty(cpu_buffer->pages));
 

Re: [PATCH] tracing: ring_buffer: wait for writers before removing pages

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] tracing: ring_buffer: ineffective synchronize_rcu() in
  rb_remove_pages()
- [Low] tracing: ring_buffer: missing Fixes trailer for
  rb_remove_pages() fix
--

commit 47d13d1f3cd83bf8e86c2d2c4091f144764c4470
Author: Haoli Zhang <26031212468@stu.xidian.edu.cn>

> tracing: ring_buffer: wait for writers before removing pages
>
> A writer can still be using the ring buffer page list when a concurrent
> resize removes and frees pages. Disabling recording prevents new writers
> from entering, but it does not wait for writers that are already in the
> reserve/commit path. The reader lock protects reader-side page operations
> and does not provide that writer-side synchronization.
>
> Disable recording before waiting for a grace period, then wait for
> pre-existing writers to finish before taking the reader lock and changing
> the page list. Keep recording disabled until after the page list has been
> updated and the reader lock has been released. The removed pages are
> unlinked and subsequently freed.
>
> This adds a grace-period wait to the resize path; it does not add
> synchronization to the normal ring-buffer write path.
>
> Reported-by: syzbot+37e1793bdb0061ea3a47@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=37e1793bdb0061ea3a47
> Co-developed-by: Jinning Zhang <25031212117@stu.xidian.edu.cn>
> Signed-off-by: Jinning Zhang <25031212117@stu.xidian.edu.cn>
> Co-developed-by: Bin Yu <byu@xidian.edu.cn>
> Signed-off-by: Bin Yu <byu@xidian.edu.cn>
> Signed-off-by: Haoli Zhang <26031212468@stu.xidian.edu.cn>

[Severity: Low]
This is a bug fix for rb_remove_pages().  Should it include:

  Fixes: 83f40318dab0 ("ring-buffer: Make removal of ring buffer pages atomic")

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index e760d2fe386fb..7ec0e12493f52 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -3080,8 +3080,11 @@ rb_remove_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned long nr_pages)
>  
>  	head_bit = 0;
>  
> -	raw_spin_lock_irq(&cpu_buffer->reader_lock);
>  	atomic_inc(&cpu_buffer->record_disabled);
> +	/* Wait for in-flight writers to finish before unlinking pages. */
> +	synchronize_rcu();
> +
> +	raw_spin_lock_irq(&cpu_buffer->reader_lock);

[Severity: Medium]
Can any writer on cpu_buffer actually be in flight when rb_remove_pages()
reaches synchronize_rcu()?

ring_buffer_lock_reserve() and ring_buffer_write() disable preemption with
preempt_disable_notrace() across the entire reserve/commit path and only
write to the local CPU's cpu_buffer.  Meanwhile, ring_buffer_resize() only
runs rb_update_pages() -> rb_remove_pages() in task context on the target
CPU itself (via schedule_work_on() or directly under migrate_disable()) or
while the CPU is offline under cpus_read_lock():

kernel/trace/ring_buffer.c:ring_buffer_resize() {
    ...
			if (!cpu_online(cpu)) {
				rb_update_pages(cpu_buffer);
				cpu_buffer->nr_pages_to_update = 0;
			} else {
				/* Run directly if possible. */
				migrate_disable();
				if (cpu != smp_processor_id()) {
					migrate_enable();
					schedule_work_on(cpu,
							 &cpu_buffer->update_pages_work);
				} else {
					update_pages_handler(&cpu_buffer->update_pages_work);
					migrate_enable();
				}
			}
    ...
}

Because rb_remove_pages() runs in task context on cpu_buffer->cpu, no
task-context writer on that CPU can be inside its preempt-disabled
reserve/commit section, and any interrupting softirq, hardirq, or NMI
writer that started before atomic_inc(&cpu_buffer->record_disabled) has
already finished before returning to task context.

Calling synchronize_rcu() here therefore does not wait for any writer.
Instead, it stalls recording on cpu_buffer for an entire RCU grace period,
blocks ring_buffer_resize() under migrate_disable() before scheduling work
on remaining CPUs, and leaves the use-after-free unfixed when
__rb_get_reader_page() leaves cpu_buffer->reader_page->list.prev pointing
to a buffer_page that rb_remove_pages() subsequently unlinks and frees
before a writer checks rb_is_reader_page() in rb_move_tail() or
rb_set_commit_to_write().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110045850.2786-1-26031212468@stu.xidian.edu.cn?part=1

Re: [PATCH] tracing: ring_buffer: wait for writers before removing pages

[Steven Rostedt]

On Tue, 10 Nov 2026 05:44:54 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [Medium] tracing: ring_buffer: ineffective synchronize_rcu() in
>   rb_remove_pages()
> - [Low] tracing: ring_buffer: missing Fixes trailer for
>   rb_remove_pages() fix
> --
> 
> commit 47d13d1f3cd83bf8e86c2d2c4091f144764c4470
> Author: Haoli Zhang <26031212468@stu.xidian.edu.cn>
> 
> > tracing: ring_buffer: wait for writers before removing pages
> >
> > A writer can still be using the ring buffer page list when a concurrent
> > resize removes and frees pages. Disabling recording prevents new writers
> > from entering, but it does not wait for writers that are already in the
> > reserve/commit path. The reader lock protects reader-side page operations
> > and does not provide that writer-side synchronization.
> >
> > Disable recording before waiting for a grace period, then wait for
> > pre-existing writers to finish before taking the reader lock and changing
> > the page list. Keep recording disabled until after the page list has been
> > updated and the reader lock has been released. The removed pages are
> > unlinked and subsequently freed.
> >
> > This adds a grace-period wait to the resize path; it does not add
> > synchronization to the normal ring-buffer write path.
> >
> > Reported-by: syzbot+37e1793bdb0061ea3a47@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=37e1793bdb0061ea3a47
> > Co-developed-by: Jinning Zhang <25031212117@stu.xidian.edu.cn>
> > Signed-off-by: Jinning Zhang <25031212117@stu.xidian.edu.cn>
> > Co-developed-by: Bin Yu <byu@xidian.edu.cn>
> > Signed-off-by: Bin Yu <byu@xidian.edu.cn>
> > Signed-off-by: Haoli Zhang <26031212468@stu.xidian.edu.cn>  
> 
> [Severity: Low]
> This is a bug fix for rb_remove_pages().  Should it include:
> 
>   Fixes: 83f40318dab0 ("ring-buffer: Make removal of ring buffer pages atomic")
> 
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index e760d2fe386fb..7ec0e12493f52 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -3080,8 +3080,11 @@ rb_remove_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned long nr_pages)
> >  
> >  	head_bit = 0;
> >  
> > -	raw_spin_lock_irq(&cpu_buffer->reader_lock);
> >  	atomic_inc(&cpu_buffer->record_disabled);
> > +	/* Wait for in-flight writers to finish before unlinking pages. */
> > +	synchronize_rcu();
> > +
> > +	raw_spin_lock_irq(&cpu_buffer->reader_lock);  
> 
> [Severity: Medium]
> Can any writer on cpu_buffer actually be in flight when rb_remove_pages()
> reaches synchronize_rcu()?
> 
> ring_buffer_lock_reserve() and ring_buffer_write() disable preemption with
> preempt_disable_notrace() across the entire reserve/commit path and only
> write to the local CPU's cpu_buffer.  Meanwhile, ring_buffer_resize() only
> runs rb_update_pages() -> rb_remove_pages() in task context on the target
> CPU itself (via schedule_work_on() or directly under migrate_disable()) or
> while the CPU is offline under cpus_read_lock():
> 
> kernel/trace/ring_buffer.c:ring_buffer_resize() {
>     ...
> 			if (!cpu_online(cpu)) {
> 				rb_update_pages(cpu_buffer);
> 				cpu_buffer->nr_pages_to_update = 0;
> 			} else {
> 				/* Run directly if possible. */
> 				migrate_disable();
> 				if (cpu != smp_processor_id()) {
> 					migrate_enable();
> 					schedule_work_on(cpu,
> 							 &cpu_buffer->update_pages_work);
> 				} else {
> 					update_pages_handler(&cpu_buffer->update_pages_work);
> 					migrate_enable();
> 				}
> 			}
>     ...
> }
> 
> Because rb_remove_pages() runs in task context on cpu_buffer->cpu, no
> task-context writer on that CPU can be inside its preempt-disabled
> reserve/commit section, and any interrupting softirq, hardirq, or NMI
> writer that started before atomic_inc(&cpu_buffer->record_disabled) has
> already finished before returning to task context.
> 
> Calling synchronize_rcu() here therefore does not wait for any writer.
> Instead, it stalls recording on cpu_buffer for an entire RCU grace period,
> blocks ring_buffer_resize() under migrate_disable() before scheduling work
> on remaining CPUs, and leaves the use-after-free unfixed when
> __rb_get_reader_page() leaves cpu_buffer->reader_page->list.prev pointing
> to a buffer_page that rb_remove_pages() subsequently unlinks and frees
> before a writer checks rb_is_reader_page() in rb_move_tail() or
> rb_set_commit_to_write().
> 

Exactly, this "fix" is bogus.

-- Steve