Re: [PATCH] panic: make force_cpu redirect work from an NMI

[Petr Mladek <pmladek@suse.com>]

Adding more people into Cc.

I wonder if smp_call_function_single_async() can be called
safely from NMI. Maybe, we should skip the redirection
completely in this case.

On Tue 2026-07-07 20:59:21, Bradley Morgan wrote:
> nmi_panic sets panic_cpu first. Bail only on a different
> owner, and hand panic_cpu to the target after the IPI.

This is quite cryptic.
Who should bail?
What is target?
What IPI?

Please, do more descriptive commit messages and better
explain the problematic situation.

> Found by sashiko [1].

Better to use:
Reported-by: Sashiko <sashiko-bot@kernel.org>

> [1]
> https://sashiko.dev/#/patchset/20260707183253.9793-1-include@grrlz.net

Closes: https://sashiko.dev/#/patchset/20260707183253.9793-1-include@grrlz.net

> Signed-off-by: Bradley Morgan <include@grrlz.net>

> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -396,8 +396,8 @@ static bool panic_try_force_cpu(const char *fmt, va_list args)
>  		return false;
>  	}
>  
> -	/* Another panic already in progress */
> -	if (panic_in_progress())
> +	/* Bail only if a different CPU is already handling it. */
> +	if (panic_in_progress() && atomic_read(&panic_cpu) != this_cpu)

There already exist panic_on_other_cpu()...

>  		return false;

This looks strange. "return false" means that vpanic() will
continue with panic. I would expect that this CPU should end up
offline.

It would actually work because vpanic() continues with:

	if (panic_try_start()) {
		/* go ahead */
	} else if (panic_on_other_cpu())
		panic_smp_self_stop();

And panic_try_start() would obviously fail while panic_on_other_cpu()
succeed.

But still, it would be more straightforward to do here: 

	/* Stop this CPU when the panic is already proceed elsewhere. */
	if (panic_on_other_cpu())
		return true;

> -	/* Another panic already in progress */
> -	if (panic_in_progress())
> +	/* Bail only if a different CPU is already handling it. */
> +	if (panic_in_progress() && atomic_read(&panic_cpu) != this_cpu)
>  
>  	/* Which CPU won the race? */
> @@ -440,6 +440,9 @@ static bool panic_try_force_cpu(const char *fmt, va_list args)
>  		return false;
>  	}
>  
> +	/* Hand panic_cpu to the target so it wins its own panic_try_start. */
> +	atomic_set(&panic_cpu, panic_force_cpu);

As I already mentioned, this is too late. The panic_force_cpu might
already be offline because it called panic_try_start() earlier it
it failed.

The main question is whether we could safely call
smp_call_function_single_async() from NMI at all.

If not then we will need to proceed panic() on this CPU.

If yes then we need to fix this in nmi_panic(). It should
call panic_try_force_cpu() first. It should call panic_try_start()
only when panic_try_force_cpu() returns false, like when
it did not forced another CPU.
r
>  	/* IPI/NMI sent, this CPU should stop */
>  	return true;

Best Regards,
Petr