re: locking/atomic: Introduce atomic_try_cmpxchg()

[Dmitry Vyukov]

Hi,

I've come across:

commit a9ebf306f52c756c4f9e50ee9a60cd6389d71344
Author: Peter Zijlstra
Date:   Wed Feb 1 16:39:38 2017 +0100
    locking/atomic: Introduce atomic_try_cmpxchg()

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:

#define __atomic64_try_cmpxchg(type, _p, _po, _n)    \
({
                \
        typeof(_po) __po = (_po);                                       \
        typeof(*(_po)) __o = *__po;                                     \
        *__po = atomic64_cmpxchg##type((_p), __o, (_n)); \
        (*__po == __o);
         \
})

In case of success it writes the same value back into *__po, but in
case of cmpxchg success we might have lose ownership of some memory
locations and potentially over what __po has pointed to. The same
holds for the re-read of *__po.
The problem is very easy to overlook in user code (not saying about
the case when developer is already familiar with different semantics
of similar functions (e.g. C/C++ atomic operations, or gcc/clang
atomic builtins)). For this reason cmpxchg implementations usually
update comparand only in case of failure. 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;                                                        \
 })

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;                                                   \
 })