Re: [PATCH v2] tracing: ring_buffer: serialize shrink with NMI readers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] tracing: ring_buffer: infinite NMI loop when empty checks
  return false on skip
- [Medium] tracing: ring_buffer: stale event pointer in
  ring_buffer_consume() on skip
- [Low] tracing: ring_buffer: missing Fixes trailer on bug-fix commit
--

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

> tracing: ring_buffer: serialize shrink with NMI readers
>
> The syzbot report shows a use-after-free in rb_is_reader_page(), reached
> from rb_move_tail(), with the freed page released by rb_remove_pages().
> Source inspection suggests that a detached reader page can retain a link
> into the range removed during a shrink. This patch addresses that possible
> stale-link path and coordinates NMI readers with page removal.
>
> When an NMI reader cannot acquire reader_lock, rb_reader_lock() falls back
> to reading the ring buffer without the lock. Gate lockless NMI readers
> while rb_remove_pages() removes pages. Track readers that entered before
> the gate and drain them before changing the page list; new NMI readers that
> fail reader_lock while the gate is closed skip that CPU. The gate and
> reader-count protocol uses ordering barriers and release/acquire accesses.
> Readers that successfully acquire reader_lock remain serialized by it.
>
> The NMI fallback increment of record_disabled is intentionally retained.
> The pre-existing fallback comment says an unlocked NMI read may corrupt the
> buffer and that recording must remain disabled afterward; the existing
> unlock path does not undo that disable. The new nmi_readers counter is
> separate: it tracks only the temporary lifetime of the lockless NMI read
> and is decremented when that read exits.
>
> During shrink, redirect reader_page->list.prev if it points to a page being
> removed. Detect a stale reader_page->list.next as well, and after reconnecting
> the ring redirect it to the surviving successor link, including the
> transferred head flag. Treat skipped empty-buffer queries as non-empty, and
> clear the timestamp output when peek or consume skips the read.
>
> Reported-by: syzbot+37e1793bdb0061ea3a47@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspotmail.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]
Since this fixes live page unlinking in rb_remove_pages(), should this
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..e947ac629e67f 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c

[ ... ]

> @@ -6416,7 +6475,7 @@ ring_buffer_consume(struct trace_buffer *buffer, int cpu, u64 *ts,
>  	struct ring_buffer_per_cpu *cpu_buffer;
>  	struct ring_buffer_event *event = NULL;
>  	unsigned long flags;
> -	bool dolock;
> +	enum rb_reader_lock_state state;
>  
>   again:
>  	/* might be called in atomic */
> @@ -6427,15 +6486,18 @@ ring_buffer_consume(struct trace_buffer *buffer, int cpu, u64 *ts,
>  
>  	cpu_buffer = buffer->buffers[cpu];
>  	local_irq_save(flags);
> -	dolock = rb_reader_lock(cpu_buffer);
> +	state = rb_reader_lock(cpu_buffer);
>  
> -	event = rb_buffer_peek(cpu_buffer, ts, lost_events);
> +	if (state != RB_READER_SKIP)
> +		event = rb_buffer_peek(cpu_buffer, ts, lost_events);
> +	else if (ts)
> +		*ts = 0;
>  	if (event) {
>  		cpu_buffer->lost_events = 0;
>  		rb_advance_reader(cpu_buffer);
>  	}
>  
> -	rb_reader_unlock(cpu_buffer, dolock);
> +	rb_reader_unlock(cpu_buffer, state);
>  	local_irq_restore(flags);
>  
>   out:

[Severity: Medium]
Can event retain a stale non-NULL pointer when ring_buffer_consume()
retries at again: and then hits RB_READER_SKIP?

event is initialized to NULL only before the again: label.  If the first
pass peeks a RINGBUF_TYPE_PADDING event and jumps back to again: at the
end of ring_buffer_consume(), and the retry sees state == RB_READER_SKIP,
event is not cleared to NULL (unlike in ring_buffer_peek()).  if (event)
then calls rb_advance_reader(cpu_buffer) without reader_lock or an
nmi_readers reference while rb_remove_pages() is unlinking and freeing
pages, and the trailing RINGBUF_TYPE_PADDING check loops back to again:.

[ ... ]

> @@ -6848,7 +6910,7 @@ bool ring_buffer_empty(struct trace_buffer *buffer)
>  {
>  	struct ring_buffer_per_cpu *cpu_buffer;
>  	unsigned long flags;
> -	bool dolock;
> +	enum rb_reader_lock_state state;
>  	bool ret;
>  	int cpu;
>  
> @@ -6856,9 +6918,10 @@ bool ring_buffer_empty(struct trace_buffer *buffer)
>  	for_each_buffer_cpu(buffer, cpu) {
>  		cpu_buffer = buffer->buffers[cpu];
>  		local_irq_save(flags);
> -		dolock = rb_reader_lock(cpu_buffer);
> -		ret = rb_per_cpu_empty(cpu_buffer);
> -		rb_reader_unlock(cpu_buffer, dolock);
> +		state = rb_reader_lock(cpu_buffer);
> +		ret = state == RB_READER_SKIP ? false :
> +			rb_per_cpu_empty(cpu_buffer);
> +		rb_reader_unlock(cpu_buffer, state);
>  		local_irq_restore(flags);
>  
>  		if (!ret)
> @@ -6878,7 +6941,7 @@ bool ring_buffer_empty_cpu(struct trace_buffer *buffer, int cpu)
>  {
>  	struct ring_buffer_per_cpu *cpu_buffer;
>  	unsigned long flags;
> -	bool dolock;
> +	enum rb_reader_lock_state state;
>  	bool ret;
>  
>  	if (!cpumask_test_cpu(cpu, buffer->cpumask))
> @@ -6886,9 +6949,10 @@ bool ring_buffer_empty_cpu(struct trace_buffer *buffer, int cpu)
>  
>  	cpu_buffer = buffer->buffers[cpu];
>  	local_irq_save(flags);
> -	dolock = rb_reader_lock(cpu_buffer);
> -	ret = rb_per_cpu_empty(cpu_buffer);
> -	rb_reader_unlock(cpu_buffer, dolock);
> +	state = rb_reader_lock(cpu_buffer);
> +	ret = state == RB_READER_SKIP ? false :
> +		rb_per_cpu_empty(cpu_buffer);
> +	rb_reader_unlock(cpu_buffer, state);
>  	local_irq_restore(flags);
>  
>  	return ret;

[Severity: High]
Does returning false from ring_buffer_empty() and ring_buffer_empty_cpu()
on RB_READER_SKIP cause ftrace_dump_one() to spin forever in NMI context?

rb_remove_pages()
  atomic_set_release(&cpu_buffer->resize_in_progress, 1)
  raw_spin_lock_irq(&cpu_buffer->reader_lock)
  <NMI>
    ftrace_dump_one()
      while (!trace_empty(&iter))
        ring_buffer_empty_cpu()  --> false (RB_READER_SKIP)
        trace_find_next_entry_inc(&iter)
          ...
            ring_buffer_peek()   --> NULL (RB_READER_SKIP)
        touch_nmi_watchdog()     <-- spins forever

rb_reader_lock() only returns RB_READER_SKIP in NMI context when
reader_lock is held and resize_in_progress is set.  If ftrace_dump_one()
interrupts rb_remove_pages() (or runs on another CPU during a panic after
stopping the resizing CPU), rb_remove_pages() cannot resume to release
reader_lock or clear resize_in_progress until the NMI dump finishes.
Because ring_buffer_empty_cpu() reports the skipped CPU buffer as
non-empty while ring_buffer_peek() returns NULL, trace_empty(&iter) stays
false and the while (!trace_empty(&iter)) loop in ftrace_dump_one() never
terminates.

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