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

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

[Haoli Zhang]

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>

---
v2:
  - Add NMI-reader gate and ordered reader-count drain.
  - Repair both detached reader-page links when they enter the shrink range.
  - Treat skipped empty queries as non-empty and clear skipped peek/consume timestamps.
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 04bb94c..f07160e 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -522,6 +522,8 @@ struct ring_buffer_per_cpu {
 	int				cpu;
 	atomic_t			record_disabled;
 	atomic_t			resize_disabled;
+	atomic_t			resize_in_progress;
+	atomic_t			nmi_readers;
 	struct trace_buffer		*buffer;
 	raw_spinlock_t			reader_lock;	/* serialize readers */
 	arch_spinlock_t			lock;
@@ -2578,6 +2580,8 @@ rb_allocate_cpu_buffer(struct trace_buffer *buffer, unsigned long nr_pages, int
 
 	cpu_buffer->cpu = cpu;
 	cpu_buffer->buffer = buffer;
+	atomic_set(&cpu_buffer->resize_in_progress, 0);
+	atomic_set(&cpu_buffer->nmi_readers, 0);
 	raw_spin_lock_init(&cpu_buffer->reader_lock);
 	lockdep_set_class(&cpu_buffer->reader_lock, buffer->reader_lock_key);
 	cpu_buffer->lock = (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED;
@@ -3074,10 +3078,18 @@ rb_remove_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned long nr_pages)
 	struct buffer_page *last_page, *first_page;
 	unsigned long nr_removed;
 	unsigned long head_bit;
+	bool reader_page_next_removed;
 	int page_entries;
 
 	head_bit = 0;
+	reader_page_next_removed = false;
 
+	/* Stop lockless NMI readers before unlinking pages. */
+	atomic_set_release(&cpu_buffer->resize_in_progress, 1);
+	/* Pair with the NMI reader's count increment and gate check. */
+	smp_mb();
+	while (atomic_read_acquire(&cpu_buffer->nmi_readers))
+		cpu_relax();
 	raw_spin_lock_irq(&cpu_buffer->reader_lock);
 	atomic_inc(&cpu_buffer->record_disabled);
 	/*
@@ -3106,6 +3118,19 @@ rb_remove_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned long nr_pages)
 	for (nr_removed = 0; nr_removed < nr_pages; nr_removed++) {
 		to_remove = rb_list_head(to_remove)->next;
 		head_bit |= (unsigned long)to_remove & RB_PAGE_HEAD;
+
+		/*
+		 * The reader page is not part of the ring buffer list, so its
+		 * previous pointer may still point to a page being removed.
+		 * rb_is_reader_page() follows this pointer, so redirect it to a
+		 * page that will remain in the ring buffer.
+		 */
+		if (cpu_buffer->reader_page->list.prev == rb_list_head(to_remove))
+			cpu_buffer->reader_page->list.prev = tail_page;
+		/* The detached reader page can also retain a stale forward link. */
+		if (rb_list_head(cpu_buffer->reader_page->list.next) ==
+		    rb_list_head(to_remove))
+			reader_page_next_removed = true;
 	}
 	/* Read iterators need to reset themselves when some pages removed */
 	cpu_buffer->pages_removed += nr_removed;
@@ -3119,6 +3144,9 @@ rb_remove_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned long nr_pages)
 	 */
 	tail_page->next = (struct list_head *)((unsigned long)next_page |
 						head_bit);
+	/* Keep the detached reader page's forward link off removed pages too. */
+	if (reader_page_next_removed)
+		cpu_buffer->reader_page->list.next = tail_page->next;
 	next_page = rb_list_head(next_page);
 	next_page->prev = tail_page;
 
@@ -3134,6 +3162,7 @@ rb_remove_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned long nr_pages)
 	/* 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_set_release(&cpu_buffer->resize_in_progress, 0);
 
 	RB_WARN_ON(cpu_buffer, list_empty(cpu_buffer->pages));
 
@@ -6293,11 +6322,18 @@ rb_iter_peek(struct ring_buffer_iter *iter, u64 *ts)
 }
 EXPORT_SYMBOL_GPL(ring_buffer_iter_peek);
 
-static inline bool rb_reader_lock(struct ring_buffer_per_cpu *cpu_buffer)
+enum rb_reader_lock_state {
+	RB_READER_LOCKED,
+	RB_READER_NMI,
+	RB_READER_SKIP,
+};
+
+static inline enum rb_reader_lock_state
+rb_reader_lock(struct ring_buffer_per_cpu *cpu_buffer)
 {
 	if (likely(!in_nmi())) {
 		raw_spin_lock(&cpu_buffer->reader_lock);
-		return true;
+		return RB_READER_LOCKED;
 	}
 
 	/*
@@ -6310,18 +6346,35 @@ static inline bool rb_reader_lock(struct ring_buffer_per_cpu *cpu_buffer)
 	 * Reading from NMI is a oneshot deal.
 	 */
 	if (raw_spin_trylock(&cpu_buffer->reader_lock))
-		return true;
+		return RB_READER_LOCKED;
+
+	/*
+	 * A resize cannot wait for an NMI that interrupted the CPU doing the
+	 * resize. Track lockless NMI readers so page removal can stop new ones
+	 * and wait for existing readers before changing the page list.
+	 */
+	atomic_inc(&cpu_buffer->nmi_readers);
+	/* Pair with the resize gate before deciding to read without the lock. */
+	smp_mb__after_atomic();
+	if (atomic_read_acquire(&cpu_buffer->resize_in_progress)) {
+		atomic_dec_return_release(&cpu_buffer->nmi_readers);
+		return RB_READER_SKIP;
+	}
 
 	/* Continue without locking, but disable the ring buffer */
 	atomic_inc(&cpu_buffer->record_disabled);
-	return false;
+
+	return RB_READER_NMI;
 }
 
 static inline void
-rb_reader_unlock(struct ring_buffer_per_cpu *cpu_buffer, bool locked)
+rb_reader_unlock(struct ring_buffer_per_cpu *cpu_buffer,
+		 enum rb_reader_lock_state state)
 {
-	if (likely(locked))
+	if (likely(state == RB_READER_LOCKED))
 		raw_spin_unlock(&cpu_buffer->reader_lock);
+	else if (state == RB_READER_NMI)
+		atomic_dec_return_release(&cpu_buffer->nmi_readers);
 }
 
 /**
@@ -6341,18 +6394,24 @@ ring_buffer_peek(struct trace_buffer *buffer, int cpu, u64 *ts,
 	struct ring_buffer_per_cpu *cpu_buffer = buffer->buffers[cpu];
 	struct ring_buffer_event *event;
 	unsigned long flags;
-	bool dolock;
+	enum rb_reader_lock_state state;
 
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
 		return NULL;
 
  again:
 	local_irq_save(flags);
-	dolock = rb_reader_lock(cpu_buffer);
-	event = rb_buffer_peek(cpu_buffer, ts, lost_events);
+	state = rb_reader_lock(cpu_buffer);
+	if (state == RB_READER_SKIP) {
+		if (ts)
+			*ts = 0;
+		event = NULL;
+	} else {
+		event = rb_buffer_peek(cpu_buffer, ts, lost_events);
+	}
 	if (event && event->type_len == RINGBUF_TYPE_PADDING)
 		rb_advance_reader(cpu_buffer);
-	rb_reader_unlock(cpu_buffer, dolock);
+	rb_reader_unlock(cpu_buffer, state);
 	local_irq_restore(flags);
 
 	if (event && event->type_len == RINGBUF_TYPE_PADDING)
@@ -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:
@@ -6843,7 +6905,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;
 
@@ -6851,9 +6913,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)
@@ -6873,7 +6936,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))
@@ -6881,9 +6944,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;

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

[sashiko-bot]

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