Re: [RFC PATCH v2 3/5] timers: Avoid blocking on hrtimer_cpu_base lock during Oops or panic

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] timers: incomplete !CONFIG_SMP guard in print_active_timers()
--

commit 8e788faa0cf2d16ee117d7196f09432346eeda90
Author: Aaron Tomlin <atomlin@xxxxxxxxxxx>

timers: Avoid blocking on hrtimer_cpu_base lock during Oops or panic

This commit switches sysrq_timer_list_show() to use ktime_get_mono_fast_ns()
and updates print_active_timers() to use raw_spin_trylock_irqsave() on
base->cpu_base->lock when in NMI, Oops, or panic contexts so that dumping
timer state does not deadlock if a CPU is holding a timekeeping or hrtimer
base lock.

> diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
> index 0406bf4488e9f..4973650c4ae5a 100644
> --- a/kernel/time/timer_list.c
> +++ b/kernel/time/timer_list.c
> @@ -68,7 +68,15 @@ static void print_active_timers(struct seq_file *m, struct hrtimer_clock_base *b
>  
>  	touch_nmi_watchdog();
>  
> -	raw_spin_lock_irqsave(&base->cpu_base->lock, flags);
> +	if (in_nmi() || oops_in_progress || panic_in_progress()) {
> +		if ((!IS_ENABLED(CONFIG_SMP) && in_nmi()) ||
> +		    !raw_spin_trylock_irqsave(&base->cpu_base->lock, flags)) {
> +			SEQ_printf(m, "  [cpu_base lock contended]\n");
> +			return;
> +		}
> +	} else {
> +		raw_spin_lock_irqsave(&base->cpu_base->lock, flags);
> +	}

[Severity: High]
In print_active_timers(), the outer check enters this branch when in_nmi(),
oops_in_progress, or panic_in_progress() is true, but the inner !CONFIG_SMP
check only tests in_nmi(). What happens on a !CONFIG_SMP build if an Oops or
panic occurs in non-NMI context while the CPU already holds
base->cpu_base->lock (for example, inside enqueue_hrtimer() or
__remove_hrtimer())?

When CONFIG_SMP=n and CONFIG_DEBUG_SPINLOCK=n, _raw_spin_trylock_irqsave()
unconditionally returns 1:

include/linux/spinlock_api_up.h:_raw_spin_trylock_irqsave() {
	__LOCK_IRQSAVE(lock, *(flags));
	return 1;
}

Wouldn't that allow print_active_timers() to re-enter the critical section
and walk base->active while the timerqueue is in a partially modified state?

And when CONFIG_SMP=n and CONFIG_DEBUG_SPINLOCK=y, since vpanic() invokes
sys_info() before debug_locks_off():

kernel/panic.c:vpanic() {
	...
	sys_info(panic_print);

	kmsg_dump_desc(KMSG_DUMP_PANIC, buf);

	if (_crash_kexec_post_notifiers)
		__crash_kexec(NULL);

	console_unblank();

	debug_locks_off();
	...
}

wouldn't do_raw_spin_trylock() hit SPIN_BUG_ON(!ret, lock, "trylock failure
on UP") when base->cpu_base->lock is already held?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103024453.922373-1-atomlin@xxxxxxxxxxx?part=3