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

[Dmitry Vyukov]

On Wed, Feb 7, 2018 at 3:17 PM, Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
> On Wed, Jan 31, 2018 at 5:17 PM, Will Deacon <will.deacon@xxxxxxx> wrote:
>>> > * Will Deacon <will.deacon@xxxxxxx> 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?


Any other comments?
Ingo, will you take this to locking tree?