lkml.org 
[lkml]   [2018]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v6 0/4] x86, kasan: add KASAN checks to atomic operations
    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?

    \
     
     \ /
      Last update: 2018-02-07 15:18    [W:3.978 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site