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

[Bradley Morgan <include@grrlz.net>]

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