[PATCH v5 0/4] panic: fix panic_force_cpu= redirect races and NMI bypass

[PATCH v5 0/4] panic: fix panic_force_cpu= redirect races and NMI bypass

[Bradley Morgan]

The panic_force_cpu= parameter redirects a panic to a specific CPU so
the crash kernel runs there. The redirect code in panic_try_force_cpu()
had two races, a va_list reuse bug and an ordering bug that bypassed
the redirect for NMI panics, all found by Sashiko. This series closes
them.

This is v5, addressing Petr's review of v4. The earlier standalone
submissions of these fixes were dropped from mm, so the series is now
self contained: the va_list fix rejoins it as patch 3.

Patch 1: fix the redirect CPU race.

The redirect is gated by an atomic cmpxchg on panic_redirect_cpu, so
only one CPU sends the redirect IPI. The cmpxchg loser used to return
false and fall through into vpanic(), where it could win
panic_try_start() and run crash_kexec on the wrong CPU before the
target ever received the IPI. The loser has to stop. It cannot just
return true, though, because panic_try_force_cpu() can be called twice
on the same CPU (nested NMI during the message formatting, before the
IPI is sent), and a blind stop on that second call would abandon the
panic with no IPI sent. The loser now returns true to stop, unless it
is reentering on the same CPU (old_cpu == this_cpu), in which case it
returns false and falls through.

Patch 1 also fixes the panic_in_progress() guard. We must never
redirect when panic_cpu is already taken, so the guard stays. But it
now returns true (stop) when the panic is on another CPU and false
(proceed) when it is on this CPU, instead of returning false either
way.

The two races side by side, two non target CPUs A and B (target is C),
then a reentry on the redirect winner:

             cpu A                          cpu B
          ----------                     ----------
    panic_try_force_cpu()                panic_try_force_cpu()
        cmpxchg wins                         cmpxchg fails
        IPI -> C                             return false  <- old BUG
        return true                      panic_try_start() wins
    panic_smp_self_stop()                 __crash_kexec() on B
    (A stops)                             (target C bypassed)

             cpu A (1st)                  cpu A (nested NMI)
          ----------                     ----------
    panic_try_force_cpu()
        cmpxchg wins (redirect = A)
        vsnprintf(msg) ...
            <-- NMI -->
                                     panic_try_force_cpu()
                                         cmpxchg fails
                                         old_cpu == A == this_cpu
                                         return true  <- would abandon
                                     self_stop (IPI never sent)

Patch 2: flatten nmi_panic control flow.

A behavior preserving cleanup. panic() is noreturn, so the else after
it is dropped and the body flattened, ready for patch 4 to add the
redirect step without piling more onto the if else chain.

Patch 3: fix va_list reuse in panic_try_force_cpu().

vsnprintf() consumes the caller's va_list. When the redirect fails,
vpanic() reuses it for the panic message, which is undefined behavior.
Fixed with va_copy(). This is the earlier standalone fix, already
reviewed by Petr, rejoining the series unchanged so patch 4 can build
on it.

Patch 4: allow force_cpu redirect from an NMI.

A panic from an NMI used to bypass the redirect entirely. nmi_panic()
called panic_try_start() first, which claims panic_cpu, so by the time
panic() reached panic_try_force_cpu() the panic_in_progress() check saw
panic_cpu set, returned false, and never sent the redirect IPI. The
crash kernel ran on the CPU that took the NMI instead of the requested
one.

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 tries the redirect before claiming panic_cpu. nmi_panic() calls
panic_try_force_cpu() first and only calls panic_try_start() when no
redirect happens. The requested CPU then claims panic_cpu itself when
its panic() runs, so panic_cpu is never handed off.

nmi_panic() receives the final message as a plain string and has no
va_list, and only a variadic function can create one. Instead of a
wrapper, panic_try_force_cpu() now takes a va_list pointer, where NULL
means that the format string already is the final message. vpanic()
hands over a disposable copy of its arguments because the address of a
va_list function parameter cannot be taken portably (on x86_64 va_list
is an array type).

The redirect IPI still goes out via smp_call_function_single_async().
Per Petr's v4 review this is not guaranteed to be safe from NMI
context. It is best effort, and worth the risk because the redirect is
only used when the crash kernel would not work on the panicking CPU
anyway.

Note: checkpatch complains about "spacing around '*'" on the new
panic_try_force_cpu() prototype. It does not recognize va_list as a
type name; the code is a regular pointer parameter.

Changes since v4:
  - Patch 1: use panic_on_other_cpu() instead of the open coded
    negation, per Petr. Added Petr's Reviewed-by.
  - Patch 2: unchanged. Added Petr's Reviewed-by.
  - Patch 3: the earlier standalone va_copy() fix rejoins the series
    unchanged, per Petr, with his earlier Reviewed-by.
  - Patch 4: dropped panic_try_force_cpu_fmt(). panic_try_force_cpu()
    takes a va_list pointer instead and nmi_panic() calls it directly,
    per Petr.
  - Patch 4: fixed a v4 bug where the unconditional self_stop dropped
    the "return when already panicking on this CPU" behavior. A nested
    NMI during panic(), for example with unknown_nmi_panic, would have
    parked the CPU and hung the interrupted panic.
  - Patch 4: the redirecting CPU now marks itself offline before
    stopping, like vpanic() does, so panic_other_cpus_shutdown() on the
    target does not wait for it.
  - Patch 4: the IPI-from-NMI justification is reworded as best
    effort, per Petr.
  - Added Fixes: tags, and Cc: stable on patches 1, 3 and 4.

Changes since v3 (v4 numbering, see the v4 cover letter):
  - Patch 1 now also fixes the panic_in_progress() guard to return
    true or false depending on which CPU owns panic_cpu, and drops the
    recursion framing in the comment per Petr's review.
  - Patch 3 no longer changes the panic_try_force_cpu() signature or
    formats the message before the redirect cmpxchg. Petr pointed out
    the static buf is only safe under panic_cpu ownership, so the
    formatting stays inside the cmpxchg guarded path.
  - Patch 3 adds the NMI safety justification for
    smp_call_function_single_async(), answering Petr's v1 question.
  - The nmi_panic() control flow cleanup is split into its own patch
    (patch 2), per Petr's request to split changes.

Bradley Morgan (4):
  panic: fix redirect CPU race in panic_try_force_cpu()
  panic: flatten nmi_panic control flow
  panic: fix va_list reuse in panic_try_force_cpu()
  panic: allow force_cpu redirect from an NMI

 kernel/panic.c | 77 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 22 deletions(-)

-- 
2.47.3

[PATCH v5 1/4] panic: fix redirect CPU race in panic_try_force_cpu()

[Bradley Morgan]

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 because the cmpxchg loser returns false and falls through
into vpanic(). Two non-target CPUs A and B panic, the requested CPU is C:

             cpu A                          cpu B
          ----------                     ----------
    panic()                              panic()
    vpanic()                             vpanic()
    panic_try_force_cpu()                panic_try_force_cpu()
        cmpxchg wins                        cmpxchg fails
        redirect = A                        old_cpu = A
        IPI -> C                            return false      <- BUG
        return true                     panic_try_start() wins
    panic_smp_self_stop()                __crash_kexec() on B
    (A stops)                            (target C bypassed)

The loser must stop, not fall through. It cannot just return true,
though. A CPU that already won the redirect cmpxchg can reenter
panic_try_force_cpu() on the same CPU, for example a nested NMI during
the message formatting, before the IPI is sent:

             cpu A (1st)                  cpu A (nested)
          ----------                     ----------
    panic()
    vpanic()
    panic_try_force_cpu()
        cmpxchg wins (redirect = A)
        vsnprintf(msg) ...
            <-- NMI, nested panic -->
                                     panic()
                                     vpanic()
                                     panic_try_force_cpu()
                                         cmpxchg fails
                                         old_cpu == A (this CPU)
                                         return true   <- would halt
                                     panic_smp_self_stop()
                                     (IPI never sent, panic abandoned)

Check old_cpu against this_cpu so a second call from the same CPU
returns false and falls through to panic_try_start() instead.

Also fix the panic_in_progress() check. We must not redirect when
panic_cpu is already assigned. Return true to stop when the panic is on
another CPU, false to proceed when it is this one.

Fixes: 2e171ab29f91 ("panic: add panic_force_cpu= parameter to redirect panic to a specific CPU")
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
Cc: stable@vger.kernel.org
Reviewed-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Bradley Morgan <include@grrlz.net>
---
 kernel/panic.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 213725b612aa..26b91b92dd71 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -396,16 +396,20 @@ static bool panic_try_force_cpu(const char *fmt, va_list args)
 		return false;
 	}
 
-	/* Another panic already in progress */
+	/*
+	 * Don't redirect when a panic is already in progress. Stop this
+	 * CPU when it's another one, proceed when it's this one.
+	 */
 	if (panic_in_progress())
-		return false;
+		return panic_on_other_cpu();
 
 	/*
-	 * 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 we already tried the redirection
+	 * from this CPU before, for example via nmi_panic().
 	 */
 	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
-- 
2.47.3

[PATCH v5 2/4] panic: flatten nmi_panic control flow

[Bradley Morgan]

panic() is __noreturn, so the else after panic_try_start() is dead.
Drop it so the force_cpu path can be added cleanly on top.

Reviewed-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Bradley Morgan <include@grrlz.net>
---
 kernel/panic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 26b91b92dd71..74065c860779 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -516,7 +516,8 @@ void nmi_panic(struct pt_regs *regs, const char *msg)
 {
 	if (panic_try_start())
 		panic("%s", msg);
-	else if (panic_on_other_cpu())
+
+	if (panic_on_other_cpu())
 		nmi_panic_self_stop(regs);
 }
 EXPORT_SYMBOL(nmi_panic);
-- 
2.47.3

[PATCH v5 3/4] panic: fix va_list reuse in panic_try_force_cpu()

[Bradley Morgan]

vsnprintf() consumes the caller's va_list. When the redirect fails,
vpanic() reuses it for the panic message, which is undefined
behavior. Use va_copy().

Fixes: 2e171ab29f91 ("panic: add panic_force_cpu= parameter to redirect panic to a specific CPU")
Cc: stable@vger.kernel.org
Reviewed-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Bradley Morgan <include@grrlz.net>
---
 kernel/panic.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 74065c860779..a483587fdd2f 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -416,7 +416,12 @@ 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) {
-		vsnprintf(panic_force_buf, PANIC_MSG_BUFSZ, fmt, args);
+		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);
 		msg = panic_force_buf;
 	} else {
 		msg = "Redirected panic (buffer unavailable)";
-- 
2.47.3

[PATCH v5 4/4] panic: allow force_cpu redirect from an NMI

[Bradley Morgan]

nmi_panic() claims panic_cpu via panic_try_start() before calling
panic(). When the panic later reaches panic_try_force_cpu(), the
panic_in_progress() check sees panic_cpu set and refuses to redirect.
The crash kernel runs on the CPU that took the NMI instead of the CPU
requested with panic_force_cpu=:

  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 the requested CPU

Try the redirect before claiming panic_cpu instead, as suggested by
Petr Mladek. nmi_panic() now calls panic_try_force_cpu() first and
claims panic_cpu only when no redirect happened. The requested CPU
claims panic_cpu itself when it runs panic(), so panic_cpu does not
need to be handed off.

nmi_panic() receives the final message as a plain string and has no
va_list. Let panic_try_force_cpu() take a va_list pointer instead,
where a NULL pointer means that @fmt already is the final message and
nothing needs to be formatted. This avoids both a variadic wrapper
and any formatting in the NMI path.

vpanic() hands over a disposable copy of its arguments because the
address of a va_list function parameter cannot be taken portably, for
example on x86_64 where va_list is an array type.

The redirect IPI is sent with smp_call_function_single_async(), which
is not guaranteed to work from NMI context. Treat it as best effort.
It is worth the risk because the redirection is only used when the
crash kernel would not work on the panicking CPU anyway.

Keep returning when the panic is already running on this CPU. A
nested NMI, for example with unknown_nmi_panic while this CPU is
inside panic(), must return and let the interrupted panic() continue
instead of parking the CPU in nmi_panic_self_stop().

Mark the redirecting CPU offline before stopping it, like vpanic()
does, so that panic_other_cpus_shutdown() on the target CPU does not
wait for it.

Fixes: 2e171ab29f91 ("panic: add panic_force_cpu= parameter to redirect panic to a specific CPU")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260708164312.19044-1-include@grrlz.net
Cc: stable@vger.kernel.org
Signed-off-by: Bradley Morgan <include@grrlz.net>
---
 kernel/panic.c | 55 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 16 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index a483587fdd2f..748773da178f 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -364,18 +364,19 @@ 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
+ * @fmt: panic message format string, or the final message when @args is NULL
+ * @args: arguments for the format string, or NULL when @fmt is final
  *
  * Some platforms require panic handling to occur on a specific CPU
  * for the crash kernel to function correctly. This function redirects
  * panic handling to the CPU specified via the panic_force_cpu= boot parameter.
  *
- * Returns false if panic should proceed on current CPU.
- * Returns true if panic was redirected.
+ * Returns true when this CPU must stop: the panic was redirected or is
+ * already running on another CPU.
+ * Returns false when panic() should proceed on this CPU.
  */
 __printf(1, 0)
-static bool panic_try_force_cpu(const char *fmt, va_list args)
+static bool panic_try_force_cpu(const char *fmt, va_list *args)
 {
 	int this_cpu = raw_smp_processor_id();
 	int old_cpu = PANIC_CPU_INVALID;
@@ -412,14 +413,18 @@ static bool panic_try_force_cpu(const char *fmt, va_list args)
 		return old_cpu != this_cpu;
 
 	/*
-	 * Use dynamically allocated buffer if available, otherwise
-	 * fall back to static message for early boot panics or allocation failure.
+	 * A NULL @args means that @fmt is already the final message, for
+	 * example from nmi_panic(). Otherwise use the dynamically allocated
+	 * buffer if available, or fall back to a static message for early
+	 * boot panics or allocation failure.
 	 */
-	if (panic_force_buf) {
+	if (!args) {
+		msg = fmt;
+	} else if (panic_force_buf) {
 		va_list ap;
 
 		/* Do not consume args, the caller reuses it if we fail */
-		va_copy(ap, args);
+		va_copy(ap, *args);
 		vsnprintf(panic_force_buf, PANIC_MSG_BUFSZ, fmt, ap);
 		va_end(ap);
 		msg = panic_force_buf;
@@ -452,7 +457,7 @@ static bool panic_try_force_cpu(const char *fmt, va_list args)
 }
 #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 *fmt, va_list *args)
 {
 	return false;
 }
@@ -512,13 +517,24 @@ bool panic_on_other_cpu(void)
 EXPORT_SYMBOL(panic_on_other_cpu);
 
 /*
- * A variant of panic() called from NMI context. We return if we've already
- * panicked on this CPU. If another CPU already panicked, loop in
- * nmi_panic_self_stop() which can provide architecture dependent code such
- * as saving register state for crash dump.
+ * A variant of panic() called from NMI context. The panic is first
+ * redirected to the CPU requested via panic_force_cpu=, when configured.
+ * We return if we've already panicked on this CPU. If another CPU already
+ * panicked, loop in nmi_panic_self_stop() which can provide architecture
+ * dependent code such as saving register state for crash dump.
  */
 void nmi_panic(struct pt_regs *regs, const char *msg)
 {
+	/* Try to redirect to the requested CPU before claiming panic_cpu. */
+	if (panic_try_force_cpu(msg, NULL)) {
+		/*
+		 * Mark ourselves offline so panic_other_cpus_shutdown() won't
+		 * wait for us on architectures that check num_online_cpus().
+		 */
+		set_cpu_online(raw_smp_processor_id(), false);
+		nmi_panic_self_stop(regs);
+	}
+
 	if (panic_try_start())
 		panic("%s", msg);
 
@@ -590,6 +606,7 @@ void vpanic(const char *fmt, va_list args)
 	long i, i_next = 0, len;
 	int state = 0;
 	bool _crash_kexec_post_notifiers = crash_kexec_post_notifiers;
+	va_list redirect_args;
 
 	if (panic_on_warn) {
 		/*
@@ -610,8 +627,13 @@ void vpanic(const char *fmt, va_list args)
 	local_irq_disable();
 	preempt_disable_notrace();
 
-	/* Redirect panic to target CPU if configured via panic_force_cpu=. */
-	if (panic_try_force_cpu(fmt, args)) {
+	/*
+	 * Redirect panic to the target CPU if configured via panic_force_cpu=.
+	 * Hand over a disposable copy of the arguments, the address of a
+	 * va_list parameter cannot be taken portably.
+	 */
+	va_copy(redirect_args, args);
+	if (panic_try_force_cpu(fmt, &redirect_args)) {
 		/*
 		 * Mark ourselves offline so panic_other_cpus_shutdown() won't wait
 		 * for us on architectures that check num_online_cpus().
@@ -619,6 +641,7 @@ void vpanic(const char *fmt, va_list args)
 		set_cpu_online(smp_processor_id(), false);
 		panic_smp_self_stop();
 	}
+	va_end(redirect_args);
 	/*
 	 * It's possible to come here directly from a panic-assertion and
 	 * not have preempt disabled. Some functions called from here want
-- 
2.47.3