Re: [PATCH v6 0/4] x86, kasan: add KASAN checks to atomic operations

From: Dmitry Vyukov <dvyukov@google.com>
To: Will Deacon <will.deacon@arm.com>
Cc: Ingo Molnar <mingo@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
	Andrey Ryabinin <aryabinin@virtuozzo.com>,
	kasan-dev <kasan-dev@googlegroups.com>,
	"the arch/x86 maintainers" <x86@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH v6 0/4] x86, kasan: add KASAN checks to atomic operations
Date: Wed, 7 Feb 2018 15:17:35 +0100	[thread overview]
Message-ID: <CACT4Y+Ymn4U7R849psS49N8LKesAEpsTBnbZbpgJsvut6CsQ6g@mail.gmail.com> (raw)
In-Reply-To: <20180131161725.GA18758@arm.com>

On Wed, Jan 31, 2018 at 5:17 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > * Will Deacon <will.deacon@arm.com> wrote:
>> >> e.g. for atomic[64]_read, your asm-generic header looks like:
>> >>
>> >> #ifndef _LINUX_ATOMIC_INSTRUMENTED_H
>> >> #define _LINUX_ATOMIC_INSTRUMENTED_H
>> >>
>> >> #include <linux/build_bug.h>
>> >> #include <linux/kasan-checks.h>
>> >>
>> >> 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?