Re: [PATCH v6 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] Incomplete panic-friendly locking leaves the system vulnerable to deadlocks during non-NMI panics.
--

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

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

This commit replaces raw_spin_lock_irqsave with rb_reader_lock in the
ring_buffer_iter functions. This prepares the iterator interface for dumping
the buffer on panic without deadlocking on the reader_lock.

> 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
[ ... ]
> @@ -6302,11 +6308,14 @@ ring_buffer_iter_peek(struct ring_buffer_iter *iter, u64 *ts)
>  	struct ring_buffer_per_cpu *cpu_buffer = iter->cpu_buffer;
>  	struct ring_buffer_event *event;
>  	unsigned long flags;
> +	bool dolock;
>  
>   again:
> -	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() actually prevent
deadlocks during non-NMI panics?

Looking at rb_reader_lock(), it only falls back to raw_spin_trylock() when
in_nmi() is true:

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;
	}
...

If a standard oops or sysrq-c triggers a panic in a normal process or softirq
context where in_nmi() is false, won't this unconditionally call
raw_spin_lock()?

If smp_send_stop() halted another CPU that was currently holding the
reader_lock, or if the panicking CPU interrupted a context holding the lock,
this would spin forever and prevent kdump or panic_timeout from working.

Should rb_reader_lock() also check oops_in_progress to fully realize the
panic-friendly locking behavior intended by this patch?

[ ... ]

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