Re: [PATCH v2] panic: allow force_cpu redirect from an NMI

[Petr Mladek <pmladek@suse.com>]

On Wed 2026-07-08 16:43:12, 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(),
> panic_in_progress() 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.
>
> smp_call_function_single_async() is safe from NMI context,

Why do you think so, please?

I was not sure and asked about this in v1, see
https://lore.kernel.org/all/ak5GGf7ypVthkZk_@pathway.suse.cz/
I haven't seen any answer or explanation.

The comment above smp_call_function_single_async() says that it
should be safe in IRQ context. It does not talk about NMI.

You might be right. But I would expect an explanation instead
of a simple claim.

> the redirect first. nmi_panic() now calls panic_try_force_cpu()
> before panic_try_start(), and only claims panic_cpu when no redirect
> is done. The requested CPU then claims panic_cpu itself.
>
> Also use panic_on_other_cpu() in place of the open coded check and
> return true to stop directly.

This is weird. This patch replaces panic_in_progress() with
panic_on_other_cpu() and the commit message should explain why.

The patch also shuffles the code formatting the panic message:

    + It should be described in the commit message.

    + It actually should be done in a separate patch. Because
      mixing too many changes into a single patch complicates
      the review and bisection of eventual regressions.

   + Most importantly, it can't be done because it is racy,
     see below.


> diff --git a/kernel/panic.c b/kernel/panic.c
> index ebdc46af6aa9..b039bb6f1d19 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -396,9 +393,9 @@ static bool panic_try_force_cpu(const char *fmt, va_list args)
>  		return false;
>  	}
>  
> -	/* Another panic already in progress */
> -	if (panic_in_progress())
> -		return false;
> +	/* Stop this CPU when the panic is already proceeding elsewhere. */
> +	if (panic_on_other_cpu())
> +		return true;

How should we handle the case when the panic is on this CPU?
Could this happen?

We should not try to redirect panic() when "panic_cpu" is already
assigned.

We should always return when panic_in_progress(). But we should return
either true or false depending whether the panic is on another or this CPU.

>  	/*
>  	 * Only one CPU can do the redirection. Others should go
> @@ -412,12 +409,7 @@ static bool panic_try_force_cpu(const char *fmt, va_list args)
>  	 * fall back to static message for early boot panics or allocation failure.
>  	 */
>  	if (panic_force_buf) {
> -		va_list ap;
> -
> -		/* Do not consume args, the caller reuses it if we fail */
> -		va_copy(ap, args);
> -		vsnprintf(panic_force_buf, PANIC_MSG_BUFSZ, fmt, ap);
> -		va_end(ap);
> +		strscpy(panic_force_buf, msg, PANIC_MSG_BUFSZ);

This removes a code added by another patch which is still in Andrew's
staging. A better solution would have been to ask Andrew to replace
the older patch with this one.

But we actually want to keep it to avoid the race, see below.

>  		msg = panic_force_buf;
>  	} else {
>  		msg = "Redirected panic (buffer unavailable)";
> @@ -515,7 +506,9 @@ EXPORT_SYMBOL(panic_on_other_cpu);
>   */
>  void nmi_panic(struct pt_regs *regs, const char *msg)
>  {
> -	if (panic_try_start())
> +	if (panic_try_force_cpu(msg))
> +		nmi_panic_self_stop(regs);
> +	else if (panic_try_start())
>  		panic("%s", msg);
>  	else if (panic_on_other_cpu())
>  		nmi_panic_self_stop(regs);

This code is kind of a puzzle. It is partly because panic_try*() does
not explain well the meaning. And partly because the return
values from both panic_try*() functions have a different
meaning.

Also the repeated nmi_panic_self_stop() looks a bit ugly.

I think that we could do better. And we could use the fact
that panic() is a no return function.

I would suggest something like:

<proposal_1>
	/* Try to redirect panic() to a requested CPU when set. */
	if (panic_try_force_cpu(msg))
		goto self_stop;

	/* Try to acquire rights to proceed with (noreturn) panic(). */
	if (panic_try_start())
		panic("%s", msg);

	 if (panic_on_other_cpu())
		goto self_stop;

	/*
	 * This should never happen. This CPU either acquired "panic_cpu"
	 * or it has already been taken in which case panic_on_other_cpu()
	 * should return true.
	 */
	return;

self_stop:
	nmi_panic_self_stop(regs);
</proposal_1>

IMHO, we actually could do:

<proposal_2>
	/* Try to redirect panic() to a requested CPU when set. */
	if (panic_try_force_cpu(msg))
		goto self_stop;

	/* Try to acquire rights to proceed with (noreturn) panic(). */
	if (panic_try_start())
		panic("%s", msg);

	/*
	 * panic_try_start() might fail only when the panic() is
	 * already in progress on another CPU in which case
	 * this CPU should stop.
	  */
self_stop:
	nmi_panic_self_stop(regs);
</proposal_2>

It would be better to split this into two patches:

   + 1st patch would remove the original if else.
   + 2nd patch would add the panic_try_force_cpu(msg) + goto.


> @@ -609,8 +602,13 @@ void vpanic(const char *fmt, va_list args)
>  	local_irq_disable();
>  	preempt_disable_notrace();
>  
> +	/* Format the message once; reused for the log and any redirect IPI. */
> +	len = vscnprintf(buf, sizeof(buf), fmt, args);
> +	if (len && buf[len - 1] == '\n')
> +		buf[len - 1] = '\0';

This is not safe. "buf" is defined as a static variable. It is _not_
on stack but it is a global variable. It can be used only by
the CPU which wins cmpxchg to "panic_cpu". IMHO, we need to keep
it as is.

> +
>  	/* Redirect panic to target CPU if configured via panic_force_cpu=. */
> -	if (panic_try_force_cpu(fmt, args)) {
> +	if (panic_try_force_cpu(buf)) {
>  		/*
>  		 * Mark ourselves offline so panic_other_cpus_shutdown() won't wait
>  		 * for us on architectures that check num_online_cpus().


More info:

This patch has been hard to review:

  1. It did not apply because it depended on two other patches.
     Only one of them was in Andrews' unstable tree.

     => It is better to send patchsets than single patches which
	depend on each other.

     => Also it is better to wait until the discussion settles so
	that you know which patches were taken and which not. [*]


  2. The patch did many things together.

     => It is always better to split changes into more patches.


  3. The commit message did not describe all the changes.

     => Double check everything before sending patches to
	the mailing list.


[*] The problem was partly also because Andrew did take the patches
    into the unstable tree so quickly.

Best Regards,
Petr