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

[sashiko-bot@kernel.org]

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