Re: [PATCH v2] panic: stop CPUs that lose the panic_redirect_cpu race

[Petr Mladek <pmladek@suse.com>]

On Tue 2026-07-07 18:32:53, Bradley Morgan wrote:
> Loser of the redirect cmpxchg must stop, not fall through to
> panic_try_start().

This described the change to "return true". It does not explain
why it was updated to "old_cpu != this_cpu" in v2.

Also it looks like a lazy description. It does not make much
sense without looking into the patch. People need to
check:

    What is "redirect cmpxchg"?
    What does panic_try_start() do?

I think that we could do much better job. See how Sashiko
tried to describe the scenario...

> This issue was found by sashiko [1].
> 
> [1] https://sashiko.dev/#/patchset/20260705164123.18746-1-include%40grrlz.net

Link to Sashiko provides more information but it is not guaranteed
to stay. The important information should be added to the commit
message.

Also it would be good add link to the 2nd report which affected
the fix as well.

Finally, it would be better to use the tags Reported-by: and Closes:

Well, I would keep this patch only for fixing the 1st problem
(race in cmpxchg panic_try_force_cpu(). I mean to use "return true".
But I would improve the commit message, something like:

<proposal>
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 in panic_try_force_cpu() because CPUs continue
with vpanic() when the cmpxchg() fails. As a result, they might
win in panic_try_start() instead of the requested CPU.

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

> Signed-off-by: Bradley Morgan <include@grrlz.net>
> ---
>  kernel/panic.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> Changes since v1: a nested panic on the CPU that already holds
> panic_redirect_cpu would fail its own cmpxchg and, with v1's
> unconditional return true, halt itself and abandon the panic.
> Check old_cpu == this_cpu so the winner of a nested reentry
> falls through to panic_try_start() instead.

This is important and should have been mentioned in the commit message.

> diff --git a/kernel/panic.c b/kernel/panic.c
> index 03f1eef07b17..e46c37b39c40 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -400,12 +400,9 @@ static bool panic_try_force_cpu(const char *fmt, va_list args)
>  	if (panic_in_progress())
>  		return false;
>  
> -	/*
> -	 * Only one CPU can do the redirect. Use atomic cmpxchg to ensure
> -	 * we don't race with another CPU also trying to redirect.
> -	 */
> +	/* Which CPU won the race? */

I do not like the new comment. What exactly is the "the race"?
And it does not explain the "old_cpu != this_cpu" check.

I would suggest to write something like, for "return true" variant:

	/*
	 * Only one CPU can do the redirection. Others should go
	 * offline.
	 */

And update it in the 2nd patch to:

	/*
	 * Only one CPU can do the redirection. Others should go
	 * offline. Continue with panic() when ending recursively here.
	 */

>  	if (!atomic_try_cmpxchg(&panic_redirect_cpu, &old_cpu, this_cpu))
> -		return false;
> +		return old_cpu != this_cpu;

Best Regards,
Petr