Re: [PATCH v3] panic: fix redirect CPU race in panic_try_force_cpu()

[Petr Mladek <pmladek@suse.com>]

On Wed 2026-07-08 16:35:32, Bradley Morgan wrote:
> The cmpxchg() in panic_try_force_cpu() makes sure that only one CPU
> tries to redirect panic() to the requested CPU. It is similar to the
> cmpxchg() in panic_try_start() which makes sure that only one CPU does
> the panic(). In both situations, only the winner of cmpxchg() should
> proceed further. Other CPUs should go offline.
> 
> There is a bug in panic_try_force_cpu() because CPUs continue with
> vpanic() when the cmpxchg() fails. As a result, they might win in
> panic_try_start() instead of the requested CPU.
> 
> The loser cannot just return true, because a CPU that already won the
> redirect cmpxchg and then reenters panic(), for example via an NMI
> during the string formatting, would fail its own cmpxchg and stop
> itself, abandoning the panic. Check old_cpu == this_cpu so a recursive
> reentry falls through to panic_try_start() instead.

The last paragraph is a bit cryptic. The panic() call in nmi_panic()
is neither reentrance nor recursion. panic_try_force_cpu() just can
be called twice on the same CPU in this code path.

Actually, it is not clear that it can be called twice
via nmi_panic() because the patch adding the panic_try_force_cpu()
there has not been accepted yet, see
https://lore.kernel.org/all/20260708164312.19044-1-include@grrlz.net/

Please, send the related fixes in a single patches so that
the ordering is clear. Otherwise, it is a mess.

Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260705164123.18746-1-include@grrlz.net
> Closes: https://sashiko.dev/#/patchset/20260707172252.4842-1-include@grrlz.net
> 
> Signed-off-by: Bradley Morgan <include@grrlz.net>
> ---
>  kernel/panic.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Changes since v2:
>   - Rewrote the commit message to describe the race and the recursive case.
>   - Used Reported-by and Closes tags.
>   - Documented the reason return old_cpu != this_cpu is needed.
> 
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 03f1eef07b17..ebdc46af6aa9 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -401,11 +401,11 @@ static bool panic_try_force_cpu(const char *fmt, va_list args)
>  		return false;
>  
>  	/*
> -	 * Only one CPU can do the redirect. Use atomic cmpxchg to ensure
> -	 * we don't race with another CPU also trying to redirect.
> +	 * Only one CPU can do the redirection. Others should go
> +	 * offline. Continue with panic() when ending recursively here.
>  	 */

I thought more about this. The recursion is not the right word here.
In fact, we should try to avoid panic() recursion because it
is likely to end up in an infinite loop. For example, see
the commit 1a2383e8b84c0451fd9 ("panic: unset panic_on_warn inside panic()").

IMHO, the panic() recursion might happen. But I think that our primary
concern is calling panic_try_force_cpu() twice on the same CPU via in nmi_panic(), see
https://lore.kernel.org/all/20260708164312.19044-1-include@grrlz.net/

A better comment might be:

	/*
	 * Only one CPU can do the redirection. Others should go offline [*].
	 * Continue with panic() when we already tried the redirection
	 * from this CPU before, for example, in nmi_panic().
	 *
	 * [*] Note that we are here only when this_cpu != panic_redirect_cpu.
	 */

But this would make sense only when nmi_panic() is updated first.
It might make sense to merge the two patches. Otherewise, there
might be an Chicken & Egg problem.

Note that this change is needed also because of the potential panic()
recursion. But I do not want to express it that much. It might make
false feeling that panic() recursion is fine.

>  	if (!atomic_try_cmpxchg(&panic_redirect_cpu, &old_cpu, this_cpu))
> -		return false;
> +		return old_cpu != this_cpu;
>  
>  	/*
>  	 * Use dynamically allocated buffer if available, otherwise

Best Regards,
Petr