linux-kernel - Re: [PATCH] panic: Use atomic_try_cmpxchg in panic() and nmi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAFULd4Yc3g1+20X6xaKmDJ_eGt17Ea=RHtKm7d21-Fci5ZbtHQ@mail.gmail.com>
Date:   Tue, 5 Sep 2023 12:11:40 +0200
From:   Uros Bizjak <ubizjak@...il.com>
To:     Mark Rutland <mark.rutland@....com>
Cc:     linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] panic: Use atomic_try_cmpxchg in panic() and nmi_panic()

On Tue, Sep 5, 2023 at 11:49 AM Mark Rutland <mark.rutland@....com> wrote:
>
> On Mon, Sep 04, 2023 at 05:21:01PM +0200, Uros Bizjak wrote:
> > Use atomic_try_cmpxchg instead of atomic_cmpxchg (*ptr, old, new) == old
> > in panic() and nmi_panic().  x86 CMPXCHG instruction returns success in
> > ZF flag, so this change saves a compare after cmpxchg (and related move
> > instruction in front of cmpxchg).
> >
> > Also, rename cpu variable to this_cpu in nmi_panic() and try to unify
> > logic flow between panic() and nmi_panic().
> >
> > No functional change intended.
>
> Do we really need to save a compare here? A panic isn't exactly a fast path,
> and robustness and code clarity is far more important than performance here.
>
> >
> > Cc: Andrew Morton <akpm@...ux-foundation.org>
> > Signed-off-by: Uros Bizjak <ubizjak@...il.com>
> > ---
> >  kernel/panic.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/kernel/panic.c b/kernel/panic.c
> > index 07239d4ad81e..8740ac65cb2c 100644
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -192,14 +192,15 @@ atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
> >   */
> >  void nmi_panic(struct pt_regs *regs, const char *msg)
> >  {
> > -     int old_cpu, cpu;
> > +     int old_cpu, this_cpu;
> >
> > -     cpu = raw_smp_processor_id();
> > -     old_cpu = atomic_cmpxchg(&panic_cpu, PANIC_CPU_INVALID, cpu);
> > +     old_cpu = PANIC_CPU_INVALID;
> > +     this_cpu = raw_smp_processor_id();
> >
> > -     if (old_cpu == PANIC_CPU_INVALID)
> > +     /* atomic_try_cmpxchg updates old_cpu on failure */
> > +     if (atomic_try_cmpxchg(&panic_cpu, &old_cpu, this_cpu))
> >               panic("%s", msg);
> > -     else if (old_cpu != cpu)
> > +     else if (old_cpu != this_cpu)
> >               nmi_panic_self_stop(regs);
> >  }
> >  EXPORT_SYMBOL(nmi_panic);
> > @@ -311,15 +312,18 @@ void panic(const char *fmt, ...)
> >        * stop themself or will wait until they are stopped by the 1st CPU
> >        * with smp_send_stop().
> >        *
> > -      * `old_cpu == PANIC_CPU_INVALID' means this is the 1st CPU which
> > -      * comes here, so go ahead.
> > +      * cmpxchg success means this is the 1st CPU which comes here,
> > +      * so go ahead.
> >        * `old_cpu == this_cpu' means we came from nmi_panic() which sets
> >        * panic_cpu to this CPU.  In this case, this is also the 1st CPU.
> >        */
> > +     old_cpu = PANIC_CPU_INVALID;
> >       this_cpu = raw_smp_processor_id();
> > -     old_cpu  = atomic_cmpxchg(&panic_cpu, PANIC_CPU_INVALID, this_cpu);
> >
> > -     if (old_cpu != PANIC_CPU_INVALID && old_cpu != this_cpu)
> > +     /* atomic_try_cmpxchg updates old_cpu on failure */
> > +     if (atomic_try_cmpxchg(&panic_cpu, &old_cpu, this_cpu))
> > +             ;
> > +     else if (old_cpu != this_cpu)
> >               panic_smp_self_stop();
>
> That empty statement is quite painful to read and would be easy to break in
> future with other changes. It'd be better to either avoid that entirely, or use
> braces, e.g.
>
>         if (!atomic_try_cmpxchg(&panic_cpu, &old_cpu, this_cpu) &&
>             old_cpu != this_cpu)
>                  panic_smp_self_stop();
>
> ... or:
>
>         if (atomic_try_cmpxchg(&panic_cpu, &old_cpu, this_cpu)) {
>                 /* do nothing */
>         } else if (old_cpu != this_cpu) {
>                 panic_smp_self_stop();
>         }
>
> The former is closer to the existing logic, so that's probably best.

The reason for the split of the conditional is the comment above the
function that says to go ahead in case old_cpu == PANIC_CPU_INVALID
(or with patch, in case cmpxchg succeeds). I think that with the split
conditional it is easier to follow the logic, so maybe this part of
the code should read:

    /* atomic_try_cmpxchg updates old_cpu on failure */
    if (atomic_try_cmpxchg(&panic_cpu, &old_cpu, this_cpu)) {
        /* go ahead */
    } else if (old_cpu != this_cpu)
        panic_smp_self_stop();

Uros.