Messages in this thread | | | From | Dmitry Vyukov <> | Date | Mon, 26 Feb 2018 13:52:10 +0100 | Subject | Re: [PATCH v6 0/4] x86, kasan: add KASAN checks to atomic operations |
| |
On Tue, Feb 20, 2018 at 11:40 AM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Wed, Feb 7, 2018 at 3:17 PM, Dmitry Vyukov <dvyukov@google.com> wrote: >> 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? > > > Any other comments? > Ingo, will you take this to locking tree?
Peter, Ingo, ping.
| |