linux-kernel - Re: locking/atomic: Introduce atomic_try

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 24 Mar 2017 12:08:32 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Andy Lutomirski <luto@...capital.net>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Lutomirski <luto@...nel.org>,
        Borislav Petkov <bp@...en8.de>,
        Brian Gerst <brgerst@...il.com>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Paul McKenney <paulmck@...ux.vnet.ibm.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: locking/atomic: Introduce atomic_try_cmpxchg()

On Fri, Mar 24, 2017 at 10:23 AM, Peter Zijlstra <peterz@...radead.org> wrote:
>
> I tried a few variants, but nothing really made it better.

So I really hate how your thing has two return values, and fakes the
second one using the pointer value. I dislike it for two different
reasons:

 - it's bad for type checking: it makes the "real" pointer value and
the "old value" pointer value both be pointers of the same type

 - I think it's part of the reason why gcc easily generates crap code.

And I think the problem is fundamental to your interface.

So how about we change the interface entirely, with the goal being
both type safety and good code generation?

In particular, let's make the interface something that might be
optimal given possible future gcc improvements, like the ability to
use "asm goto" with outputs. We can't do that today, but maybe some
day...

So I would suggest that the format of the "try_cmpxhg()" thing be
something like this:

#define try_cmpxchg(ptr, value, new, success_label) ({  \
        bool __txchg_success;                           \
        __typeof__(*(ptr)) __old;                       \
        asm volatile("lock cmpxchgl %3, %1"             \
                : "=@ccz" (__txchg_success),            \
                  "+m" (*ptr),                          \
                  "=a" (__old)                          \
                : "r" (new),                            \
                  "2" (value)                           \
                : "memory");                            \
        if (__txchg_success) goto success_label;        \
        __old; })

which seems to be fairly natural.

Then you do your refcount loop (or pretty much *any* cmpxchg loop) with

        for (;;) {
                if (unlikely(val == UINT_MAX))
                        goto saturated;

                if (unlikely(!val))
                        goto use_after_free;

                new = val + 1;
                val = try_cmpxchg(r, val, new, success);
        }

    success:
        return;

    saturated: use_after_free: .. whatever error handling ..

and I think gcc should generate reasonable code.

Hmm?

NOTE! I would suggest that this odd "macro with a success label" model
of try_cmpxchg() never be used for something that people are expected
to use directly. So I don't think "normal" code should use this label
form.

But it's useful as a helper function that is then used to implement
the "real" ABI, ie to implement that "refcount_inc()" and other things
like that..

                               Linus