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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ