Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935407AbdCXMoc (ORCPT ); Fri, 24 Mar 2017 08:44:32 -0400 Received: from mail-vk0-f47.google.com ([209.85.213.47]:33792 "EHLO mail-vk0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935162AbdCXMoW (ORCPT ); Fri, 24 Mar 2017 08:44:22 -0400 MIME-Version: 1.0 From: Dmitry Vyukov Date: Fri, 24 Mar 2017 13:44:00 +0100 Message-ID: Subject: re: locking/atomic: Introduce atomic_try_cmpxchg() To: Peter Zijlstra , Andrew Morton , Andy Lutomirski , Borislav Petkov , Brian Gerst , Denys Vlasenko , "H. Peter Anvin" , Josh Poimboeuf , Linus Torvalds , Paul McKenney , Thomas Gleixner , Ingo Molnar , LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3346 Lines: 72 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; \ })