From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f200.google.com (mail-wr0-f200.google.com [209.85.128.200]) by kanga.kvack.org (Postfix) with ESMTP id D791C6B0292 for ; Wed, 28 Jun 2017 09:55:21 -0400 (EDT) Received: by mail-wr0-f200.google.com with SMTP id x23so33585960wrb.6 for ; Wed, 28 Jun 2017 06:55:21 -0700 (PDT) Received: from Galois.linutronix.de (Galois.linutronix.de. [2a01:7a0:2:106d:700::1]) by mx.google.com with ESMTPS id m203si5540942wma.47.2017.06.28.06.55.16 for (version=TLS1_2 cipher=AES128-SHA bits=128/128); Wed, 28 Jun 2017 06:55:16 -0700 (PDT) Date: Wed, 28 Jun 2017 15:54:42 +0200 (CEST) From: Thomas Gleixner Subject: Re: [PATCH] locking/atomics: don't alias ____ptr In-Reply-To: Message-ID: References: <85d51d3551b676ba1fc40e8fbddd2eadd056d8dd.1498140838.git.dvyukov@google.com> <20170628100246.7nsvhblgi3xjbc4m@breakpoint.cc> <1c1cbbfb-8e34-dd33-0e73-bbb2a758e962@virtuozzo.com> <20170628121246.qnk2csgzbgpqrmw3@linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: owner-linux-mm@kvack.org List-ID: To: Sebastian Andrzej Siewior Cc: Andrey Ryabinin , Ingo Molnar , Dmitry Vyukov , Mark Rutland , Peter Zijlstra , Will Deacon , "H. Peter Anvin" , kasan-dev , "x86@kernel.org" , LKML , Andrew Morton , "linux-mm@kvack.org" , Linus Torvalds On Wed, 28 Jun 2017, Thomas Gleixner wrote: > On Wed, 28 Jun 2017, Sebastian Andrzej Siewior wrote: > > On 2017-06-28 14:15:18 [+0300], Andrey Ryabinin wrote: > > > The main problem here is that arch_cmpxchg64_local() calls cmpxhg_local() instead of using arch_cmpxchg_local(). > > > > > > So, the patch bellow should fix the problem, also this will fix double instrumentation of cmpcxchg64[_local](). > > > But I haven't tested this patch yet. > > > > tested, works. Next step? > > Check all other implementations in every architecture whether there is a > similar problem ..... > > But this really want's a proper cleanup unless we want to waste the time > over and over again with the next hard to figure out macro expansion fail. > > First of all, cmpxchg64[_local]() can be implemented as inlines right away. > > For cmpxchg*(), the situation is slightly different, but the sizeof() > evaluation should be done at the top most level, even if we do it further > down in the low level arch/asm-generic implementation once more. > > Something along the lines of: > > static inline unsigned long cmpxchg_varsize(void *ptr, unsigned long old, > unsigned long new, int size) > { > switch (size) { > case 1: > case 2: > case 4: > break; > case 8: > if (sizeof(unsigned long) == 8) > break; > default: > BUILD_BUG_ON(1); > } > kasan_check(ptr, size); > return arch_cmpxchg(ptr, old, new); > } > > #define cmpxchg(ptr, o, n) \ > ({ \ > ((__typeof__(*(ptr)))cmpxchg_varsize((ptr), (unsigned long)(o), \ > (unsigned long)(n), sizeof(*(ptr)))); \ > }) > > That's the first step to cure the actual mess. > > Ideally we get rid of that whole macro maze and convert everything to > proper inlines with actual cmpxchg8/16/32/64() variants, but that's going > to take some time. As an intermediate step we can at least propagate 'size' > to arch_cmpxchg(), which is not that much of an effort. And to be honest. That should have be done in the first place _BEFORE_ adding that atomic-instrumented stuff. I'm tempted to revert that mess instead of 'fixing' it half arsed. As a side note, we have files (aside of x86/asm/atomic.h) which include asm/cmpxchg.h ... net/sunrpc/xprtmultipath.c:#include arch/x86/kvm/mmu.c:#include arch/x86/um/asm/barrier.h:#include I'm really tired of all this featuritis crammed into the code without much thought. Dammit, can we please stop this and clean up the existing mess first before duct taping more mess on top of it. Thanks, tglx -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org