Re: [PATCH v5 10/18] ring-buffer: Use panic-friendly locking in ring_buffer_iter interface

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] rb_reader_lock() lacks an oops_in_progress check, leading to deadlocks during non-NMI panic dumps.
--

--- Patch [10]: [PATCH v5 10/18] ring-buffer: Use panic-friendly locking in ring_buffer_iter interface ---
commit 15fbd37892d8549b59334542a9c228e531aa61b5
Author: Vincent Donnefort <[email protected]>

ring-buffer: Use panic-friendly locking in ring_buffer_iter interface

This patch replaces raw_spin_lock_irqsave() with rb_reader_lock() in the
iterator functions. The goal is to make the iterators panic-friendly so
trace_remote can dump the buffer on panic without deadlocking.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index a7d9936b4c835..a6f2293f409b8 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
[ ... ]
> @@ -5616,15 +5619,18 @@ void ring_buffer_iter_reset(struct ring_buffer_iter *iter)
>  {
>  	struct ring_buffer_per_cpu *cpu_buffer;
>  	unsigned long flags;
> +	bool dolock;
>  
>  	if (!iter)
>  		return;
>  
>  	cpu_buffer = iter->cpu_buffer;
>  
> -	raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
> +	local_irq_save(flags);
> +	dolock = rb_reader_lock(cpu_buffer);

[Severity: High]
This is a pre-existing issue, but does rb_reader_lock() lack an
oops_in_progress check?

If a panic occurs in a normal (non-NMI) context, in_nmi() evaluates to false
in rb_reader_lock():

static inline bool rb_reader_lock(struct ring_buffer_per_cpu *cpu_buffer)
{
	if (likely(!in_nmi())) {
		raw_spin_lock(&cpu_buffer->reader_lock);
		return true;
	}

During a panic, smp_send_stop() will forcefully halt other CPUs. If one of
those stopped CPUs was holding cpu_buffer->reader_lock, the panicking CPU
will spin forever in raw_spin_lock().

Since the goal here is to make these iterators panic-friendly, does
rb_reader_lock() also need to bypass the lock (e.g. check oops_in_progress) 
to avoid deadlocks during non-NMI panics?

>  	rb_iter_reset(iter);
> -	raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
> +	rb_reader_unlock(cpu_buffer, dolock);
> +	local_irq_restore(flags);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10