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

[Will Deacon]

On Wed, Jan 31, 2018 at 09:53:10AM +0100, Dmitry Vyukov wrote:
> On Wed, Jan 31, 2018 at 8:28 AM, Ingo Molnar <mingo@xxxxxxxxxx> 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.

Will