Re: [PATCH v6 1/6] panic: fix redirect CPU race in panic_try_force_cpu()
Petr Mladek <[email protected]>
| Newsgroups | org.kernel.vger.stable,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
On Tue 2026-08-18 16:38:01, Bradley Morgan wrote: > 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. > > Update the panic_try_force_cpu() doc comment for the new return value > semantics. > > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -396,16 +397,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 Just for completeness. Sashiko AI points out that the vsnprintf() in panic_try_force_cpu() uses the "args" and they might later be used again when the redirection fails, see https://sashiko.dev/#/patchset/20260818163806.17460-1-include%40grrlz.net This problem is fixed in by the 3rd patch in this patchset, see https://lore.kernel.org/all/[email protected]/ So, we are on the safe side. Best Regards, Petr