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

[Petr Mladek <pmladek@suse.com>]

On Sun 2026-07-26 19:04:12, Bradley Morgan wrote:
> 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.
> 
> --- 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.

Nit: This a good improvement. But it would better fit into the 1st
     patch which fixed the true/false return values.

>   */
>  __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;

Ah, I misunderstood the meaning of panic_try_force_cpu_fmt() in v4.
I though that it was needed to _copy_ va_list() but it was needed
to _create_ it.

It looks like a cleaner solution after all. But I think that
we could do even better, see below.

> +	} 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);

This is weird. panic_try_force_cpu() does its own copy when
it prints the string.

Also the comment says that it creates a copy because the address
of a va_list could not be taken portably. But the code below
passes the address of "redirect_args" which is "va_list" again.
So, the copy does not make any difference.

> +	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().

It would be better to avoid passing the pointer.

The main problem here is that nmi_panic() does not support variable
list of arguments. I see two solutions:

Solution A:

   Create a wrapper for panic_try_force_cpu() like in v4. But
   it should follow the existing naming scheme panic() vs. vpanic():

    1. Rename panic_try_force_cpu() -> vpanic_try_force_cpu()
       for the variant with va_list.

    2. Create the wrapper panic_try_force_cpu() with "..."
       variable list of parameters.


Solution B:

   Allow to pass the variable list of arguments to nmi_panic().

   Note that the original nmi_panic() accepted variable list
   of arguments. This ability was removed by the commit
   ebc41f20d77f6ad91 ("panic: change nmi_panic from macro to
   function"). But vpanic() did not exist at that time.

   I have updated this patch accordingly to see how it would look like:

diff --git a/include/linux/panic.h b/include/linux/panic.h
index f1dd417e54b2..a9128bf3c168 100644
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -13,7 +13,8 @@ __printf(1, 2)
 void panic(const char *fmt, ...) __noreturn __cold;
 __printf(1, 0)
 void vpanic(const char *fmt, va_list args) __noreturn __cold;
-void nmi_panic(struct pt_regs *regs, const char *msg);
+__printf(2, 3)
+void nmi_panic(struct pt_regs *regs, const char *fmt, ...);
 void check_panic_on_warn(const char *origin);
 extern void oops_enter(void);
 extern void oops_exit(void);
diff --git a/kernel/panic.c b/kernel/panic.c
index a483587fdd2f..7ce252268714 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -371,8 +371,9 @@ int __weak panic_smp_redirect_cpu(int target_cpu, void *msg)
  * 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)
@@ -452,7 +453,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,18 +513,37 @@ 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)
+__printf(2, 3)
+void nmi_panic(struct pt_regs *regs, const char *fmt, ...)
 {
+	va_list args;
+
+	va_start(args, fmt);
+
+	/* Try to redirect to the requested CPU before claiming panic_cpu. */
+	if (panic_try_force_cpu(fmt, args)) {
+		/*
+		 * 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);
+		vpanic(fmt, args);
 
 	if (panic_on_other_cpu())
 		nmi_panic_self_stop(regs);
+
+	va_end(args);
+
 }
 EXPORT_SYMBOL(nmi_panic);
 
I personally prefer the variant B.

Note that the above POC patch would need some changes:

  1. Move the comment changes above panic_try_force_cpu()
     into the 1st patch

  2. Add the "..." into nmi_panic() in a separate patch.

  3. The patch allowing redirection from nmi_panic() would just add
     the following into nmi_panic().

	/* Try to redirect to the requested CPU before claiming panic_cpu. */
	if (panic_try_force_cpu(fmt, args)) {
		/*
		 * 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);
	}

      plus it could the comment above nmi_panic().


Best Regards,
Petr