From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754286AbeBGOR6 (ORCPT ); Wed, 7 Feb 2018 09:17:58 -0500 Received: from mail-pl0-f51.google.com ([209.85.160.51]:37178 "EHLO mail-pl0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753872AbeBGOR4 (ORCPT ); Wed, 7 Feb 2018 09:17:56 -0500 X-Google-Smtp-Source: AH8x227J4VbZjhHhhwJA/XVCn2PtFpfEjeGDknHlPR1G9W6wA6iUdo1GOXZ34bzMznXf5rrDLuoxsDWAJDH0GFzKqa8= MIME-Version: 1.0 In-Reply-To: <20180131161725.GA18758@arm.com> References: <20180130153609.GA10917@arm.com> <20180131072858.5hoy6wsukda27v2o@gmail.com> <20180131161725.GA18758@arm.com> From: Dmitry Vyukov Date: Wed, 7 Feb 2018 15:17:35 +0100 Message-ID: Subject: Re: [PATCH v6 0/4] x86, kasan: add KASAN checks to atomic operations To: Will Deacon Cc: Ingo Molnar , Mark Rutland , Peter Zijlstra , Ingo Molnar , "H. Peter Anvin" , Andrey Ryabinin , kasan-dev , "the arch/x86 maintainers" , LKML , Thomas Gleixner Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jan 31, 2018 at 5:17 PM, Will Deacon wrote: >> > * Will Deacon wrote: >> >> e.g. for atomic[64]_read, your asm-generic header looks like: >> >> >> >> #ifndef _LINUX_ATOMIC_INSTRUMENTED_H >> >> #define _LINUX_ATOMIC_INSTRUMENTED_H >> >> >> >> #include >> >> #include >> >> >> >> static __always_inline int __atomic_read_instrumented(const atomic_t *v) >> >> { >> >> kasan_check_read(v, sizeof(*v)); >> >> return atomic_read(v); >> >> } >> >> >> >> static __always_inline s64 __atomic64_read_instrumented(const atomic64_t *v) >> >> { >> >> kasan_check_read(v, sizeof(*v)); >> >> return atomic64_read(v); >> >> } >> >> >> >> #undef atomic_read >> >> #undef atomic64_read >> >> >> >> #define atomic_read __atomic_read_instrumented >> >> #define atomic64_read __atomic64_read_instrumented >> >> >> >> #endif /* _LINUX_ATOMIC_INSTRUMENTED_H */ >> >> >> >> and the arch code just includes that in asm/atomic.h once it's done with >> >> its definitions. >> >> >> >> What do you think? Too stinky? >> > >> > Hm, so while this could work - I actually *like* the low level changes: they are >> > straightforward, trivial, easy to read and they add the arch_ prefix that makes it >> > abundantly clear that this isn't the highest level interface. >> > >> > The KASAN callbacks in the generic methods are also abundantly clear and very easy >> > to read. I could literally verify the sanity of the series while still being only >> > half awake. ;-) >> > >> > Also note that the arch renaming should be 'trivial', in the sense that any >> > missing rename results in a clear build breakage. Plus any architecture making use >> > of this new KASAN feature should probably be tested before it's enabled - and the >> > renaming of the low level atomic APIs kind of forces that too. >> > >> > So while this approach creates some churn, this series is IMHO a marked >> > improvement over the previous iterations. >> >> >> I think I mildly leaning towards Ingo's point. >> I guess people will first find the version in arch (because that's >> where they used to be), but that version is actually not the one that >> is called. >> The renaming is mechanical and you get build errors if anything is >> wrong. It's macros that caused hard to debug runtime crashes and >> multiple revisions of this series. > > Sure, and it sounds like you're proposing to do the arm64 changes anyway so > I'm not complaining! Just thought I'd float the alternative to see what > people think. Any other comments? Ingo, will you take this to locking tree?