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

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

[Bradley Morgan]

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, so move
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.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260707183253.9793-1-include@grrlz.net

Signed-off-by: Bradley Morgan <include@grrlz.net>
---
 kernel/panic.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

Changes since v1:
  - Dropped the atomic_set panic_cpu hand off, it was racy.
  - Restructured nmi_panic() to call panic_try_force_cpu() first.
  - Used panic_on_other_cpu() instead of an open coded check.
  - panic_try_force_cpu() now takes a formatted message string.

diff --git a/kernel/panic.c b/kernel/panic.c
index ebdc46af6aa9..b039bb6f1d19 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -364,8 +364,7 @@ int __weak panic_smp_redirect_cpu(int target_cpu, void *msg)
 
 /**
  * panic_try_force_cpu - Redirect panic to a specific CPU for crash kernel
- * @fmt: panic message format string
- * @args: arguments for format string
+ * @msg: already formatted panic message
  *
  * Some platforms require panic handling to occur on a specific CPU
  * for the crash kernel to function correctly. This function redirects
@@ -374,12 +373,10 @@ int __weak panic_smp_redirect_cpu(int target_cpu, void *msg)
  * Returns false if panic should proceed on current CPU.
  * Returns true if panic was redirected.
  */
-__printf(1, 0)
-static bool panic_try_force_cpu(const char *fmt, va_list args)
+static bool panic_try_force_cpu(const char *msg)
 {
 	int this_cpu = raw_smp_processor_id();
 	int old_cpu = PANIC_CPU_INVALID;
-	const char *msg;
 
 	/* Feature not enabled via boot parameter */
 	if (panic_force_cpu < 0)
@@ -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;
 
 	/*
 	 * 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);
 		msg = panic_force_buf;
 	} else {
 		msg = "Redirected panic (buffer unavailable)";
@@ -447,8 +439,7 @@ static bool panic_try_force_cpu(const char *fmt, va_list args)
 	return true;
 }
 #else
-__printf(1, 0)
-static inline bool panic_try_force_cpu(const char *fmt, va_list args)
+static inline bool panic_try_force_cpu(const char *msg)
 {
 	return false;
 }
@@ -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);
@@ -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';
+
 	/* 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().
@@ -641,10 +639,6 @@ void vpanic(const char *fmt, va_list args)
 
 	console_verbose();
 	bust_spinlocks(1);
-	len = vscnprintf(buf, sizeof(buf), fmt, args);
-
-	if (len && buf[len - 1] == '\n')
-		buf[len - 1] = '\0';
 
 	pr_emerg("Kernel panic - not syncing: %s\n", buf);
 	/*
-- 
2.53.0

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

[Petr Mladek]

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

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

[Andrew Morton]

On Mon, 13 Jul 2026 16:04:05 +0200 Petr Mladek <pmladek@suse.com> wrote:

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

This is clearly turning into a project.  I'll drop "panic: fix va_list
reuse in panic_try_force_cpu()".  Let's aim for a full standalone
series of panic fixups?

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

[Petr Mladek]

On Mon 2026-07-13 17:21:50, Andrew Morton wrote:
> On Mon, 13 Jul 2026 16:04:05 +0200 Petr Mladek <pmladek@suse.com> wrote:
> 
> > 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.
> 
> This is clearly turning into a project.  I'll drop "panic: fix va_list
> reuse in panic_try_force_cpu()".  Let's aim for a full standalone
> series of panic fixups?

Sounds good to me. It would be better to sort the problems in a
single patchset.

Best Regards,
Petr