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]

Message-ID: <20170324142140.vpyzl755oj6rb5qv@hirez.programming.kicks-ass.net>
Date:   Fri, 24 Mar 2017 15:21:40 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Dmitry Vyukov <dvyukov@...gle.com>
Cc:     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>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        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 01:44:00PM +0100, Dmitry Vyukov wrote:
> 
> The primitive has subtle difference with all other implementation that
> I know of, and can lead to very subtle bugs. Some time ago I've spent
> several days debugging a memory corruption caused by similar
> implementation. Consider a classical lock-free stack push:
> 
> node->next = atomic_read(&head);
> do {
> } while (!atomic_try_cmpxchg(&head, &node->next, node));
> 
> This code is broken with the current implementation, the problem is
> with unconditional update of *__po here:

Indeed. I had only considered stack local variables when I wrote that.

> So I would suggest to change it to a safer and less surprising
> alternative:
> 
> diff --git a/arch/x86/include/asm/cmpxchg.h b/arch/x86/include/asm/cmpxchg.h
> index fb961db51a2a..81fb985f51f4 100644
> --- a/arch/x86/include/asm/cmpxchg.h
> +++ b/arch/x86/include/asm/cmpxchg.h
> @@ -212,7 +212,8 @@ extern void __add_wrong_size(void)
>         default:                                                        \
>                 __cmpxchg_wrong_size();                                 \
>         }                                                               \
> -       *_old = __old;                                                  \
> +       if (!success)                                                   \
> +               *_old = __old;                                          \
>         success;                                                        \
>  })

I've no immediate objection, I'll double check what, if anything, it
does for code gen.

> diff --git a/include/linux/atomic.h b/include/linux/atomic.h
> index aae5953817d6..f8098157f7c8 100644
> --- a/include/linux/atomic.h
> +++ b/include/linux/atomic.h
> @@ -1023,8 +1023,11 @@ static inline int atomic_dec_if_positive(atomic_t *v)
>  ({                                                                     \
>         typeof(_po) __po = (_po);                                       \
>         typeof(*(_po)) __o = *__po;                                     \
> -       *__po = atomic64_cmpxchg##type((_p), __o, (_n));                \
> -       (*__po == __o);                                                 \
> +       typeof(*(_po)) __v = atomic64_cmpxchg##type((_p), __o, (_n));   \
> +       if (__v == __o)                                                 \
> +               return true;                                            \
> +       *__po = __v;                                                    \
> +       return false;                                                   \
>  })

Can you actually use return in statement-expressions?