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

[Steven Rostedt <rostedt@goodmis.org>]

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