Re: [PATCH v4 3/3] panic: allow force_cpu redirect from an NMI

[Petr Mladek]

On Tue 2026-07-14 17:31:02, Bradley Morgan wrote:
> nmi_panic() calls panic_try_start() before panic(), so it claims
> panic_cpu first. When the panic then reaches panic_try_force_cpu(),
> the panic_in_progress() check sees panic_cpu set and returns false,
> so the redirect to the requested CPU never happens. The crash kernel
> runs on the CPU that took the NMI instead.
> 
> The buggy call order, on a CPU X that is not the target (target is C):
> 
>   nmi_panic()
>     panic_try_start()              wins, panic_cpu = X
>     panic("%s", msg)
>       vpanic()
>         panic_try_force_cpu()
>           panic_in_progress()      true, panic_cpu is X
>           return false             redirect bypassed
>         panic_try_start()          already won
>         __crash_kexec()            on X, not C
> 
> The fix is to try the redirect before claiming panic_cpu. nmi_panic()
> now calls panic_try_force_cpu_fmt() first, and only calls
> panic_try_start() when no redirect happens. The requested CPU then
> claims panic_cpu itself when it runs panic(), so panic_cpu does not need
> to be handed off.
> 
> nmi_panic() holds an already formatted string, not a va_list. Add a

This is confusing. nmi_panic() does not _hold_ any string. Also
it is not _already_ formatted.

The problem is that the same args might be used twice in
panic_try_force_cpu() and vpanic(). So that panic_try_force_cpu()
must copy them.

I suggest to fix this in a separate patch, see below.

> variadic wrapper, panic_try_force_cpu_fmt(), so it can reach the
> existing formatting guarded by the cmpxchg in panic_try_force_cpu() without
> a signature change. The wrapper builds the va_list and the real
> function still copies and formats under the redirect cmpxchg, so no
> shared buffer is written before ownership.
> 
> The redirect sends the IPI via smp_call_function_single_async().
> This is safe from NMI context: the doc on the _async variant
> states it can be called with interrupts disabled, and kgdb_roundup_cpus()
> already calls it from NMI/debug context (kernel/debug/debug_core.c).

I do not think that it is fully safe. For example, see smp_send_stop()
vs crash_smp_send_stop().

I would rather say that it is a best effort. And it is worth the risk
because the redirection should be used only when panic() would fail
otherwise.

> The nmi_panic() body is reshaped to a goto self_stop, since panic()
> is noreturn and the stop path is shared.
> 
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -450,12 +450,32 @@ static bool panic_try_force_cpu(const char *fmt, va_list args)
>  	/* IPI/NMI sent, this CPU should stop */
>  	return true;
>  }
> +
> +/* For callers without a va_list, such as nmi_panic(). */
> +static __printf(1, 2)
> +bool panic_try_force_cpu_fmt(const char *fmt, ...)
> +{
> +	va_list args;
> +	bool ret;
> +
> +	va_start(args, fmt);
> +	ret = panic_try_force_cpu(fmt, args);
> +	va_end(args);
> +
> +	return ret;
> +}
>  #else
>  __printf(1, 0)
>  static inline bool panic_try_force_cpu(const char *fmt, va_list args)
>  {
>  	return false;
>  }
> +
> +static __printf(1, 2)
> +bool panic_try_force_cpu_fmt(const char *fmt, ...)
> +{
> +	return false;
> +}
>  #endif /* CONFIG_SMP && CONFIG_CRASH_DUMP */
>  
>  bool panic_try_start(void)
> @@ -519,11 +539,20 @@ EXPORT_SYMBOL(panic_on_other_cpu);
>   */
>  void nmi_panic(struct pt_regs *regs, const char *msg)
>  {
> +	/* Try to redirect to the requested CPU when one is set. */
> +	if (panic_try_force_cpu_fmt("%s", msg))
> +		goto self_stop;

This correctly copies the argumetns when panic_try_force_cpu()
is called in nmi_panic() code path. But there is the same
problem when panic_try_force_cpu() is called in vpanic().

IMHO, panic_try_force_cpu_fmt() makes things too complicated.
The patch where the arguments were copied directly in
panic_try_force_cpu() looked better, see
https://lore.kernel.org/all/20260705164123.18746-1-include@xxxxxxxxx/

We will need v5. Please, remove panic_try_force_cpu() in this patch.
And add the va_copy() into panic_try_force_cpu() in a separate patch
like it was proposed before. So, v5 would have 4 patches...

But please wait few days. People might have different option
about using smp_call_function_single_async() for the redirection
in NMI. Also I might have missed something....

> +
> +	/* Try to acquire the right to proceed with the noreturn panic(). */
>  	if (panic_try_start())
>  		panic("%s", msg);
>  
> -	if (panic_on_other_cpu())
> -		nmi_panic_self_stop(regs);
> +	/*
> +	 * panic_try_start() only fails when a panic is already in progress
> +	 * on another CPU, in which case this CPU must stop.
> +	 */
> +self_stop:
> +	nmi_panic_self_stop(regs);
>  }
>  EXPORT_SYMBOL(nmi_panic);

Best Regards,
Petr