Re: [tip:locking/urgent] compiler, atomics: Provide READ_ONCE_NOCHECK ()

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Wed, Oct 14, 2015 at 05:50:34PM +0200, Dmitry Vyukov wrote:
> On Wed, Oct 14, 2015 at 5:45 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Oct 14, 2015 at 08:28:43AM -0700, tip-bot for Andrey Ryabinin wrote:
> >> Commit-ID:  4115ffdf4d6f8986a7abe1dd522c163f599bc0e6
> >> Gitweb:     http://git.kernel.org/tip/4115ffdf4d6f8986a7abe1dd522c163f599bc0e6
> >> Author:     Andrey Ryabinin <aryabinin@virtuozzo.com>
> >> AuthorDate: Tue, 13 Oct 2015 18:28:07 +0300
> >> Committer:  Ingo Molnar <mingo@kernel.org>
> >> CommitDate: Wed, 14 Oct 2015 16:44:06 +0200
> >>
> >> compiler, atomics: Provide READ_ONCE_NOCHECK()
> >>
> >> Some code may perform racy by design memory reads. This could be
> >> harmless, yet such code may produce KASAN warnings.
> >>
> >> To hide such accesses from KASAN this patch introduces
> >> READ_ONCE_NOCHECK() macro. KASAN will not check the memory
> >> accessed by READ_ONCE_NOCHECK().
> >>
> >> This patch creates __read_once_size_nocheck() a clone of
> >> __read_once_size_check() (renamed __read_once_size()).
> >> The only difference between them is 'no_sanitized_address'
> >> attribute appended to '*_nocheck' function. This attribute tells
> >> the compiler that instrumentation of memory accesses should not
> >> be applied to that function. We declare it as static
> >> '__maybe_unsed' because GCC is not capable to inline such
> >> function: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
> >>
> >> With KASAN=n READ_ONCE_NOCHECK() is just a clone of READ_ONCE().
> >
> > So I add READ_ONCE_NOCHECK() for accesses for which the compiler cannot
> > prove safe address for KASAN's benefit, but READ_ONCE() suffices for
> > the data-race-detection logic in KTSAN, correct?
> 
> KTSAN also needs READ_ONCE_NOCHECK() here. KTSAN will flag races
> between get_wchan() and the thread accesses to own stack even more
> aggressively than KASAN, because KTSAN won't like get_wchan() accesses
> even to non-poisoned areas of other thread stack.

So to keep KTSAN happy, any read from some other thread's stack requires
READ_ONCE_NOCHECK()?  What if the access is via a locking primitive or
read-modify-write atomic operation?

This is of some interest in RCU, which implements synchronous grace
periods using completions that are allocated on the calling task's stack
and manipulated by RCU callbacks that are likely executing elsewhere.

							Thanx, Paul

> >> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> >> Cc: Alexander Potapenko <glider@google.com>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >> Cc: Andrey Konovalov <andreyknvl@google.com>
> >> Cc: Andy Lutomirski <luto@amacapital.net>
> >> Cc: Borislav Petkov <bp@alien8.de>
> >> Cc: Denys Vlasenko <dvlasenk@redhat.com>
> >> Cc: Dmitry Vyukov <dvyukov@google.com>
> >> Cc: Kostya Serebryany <kcc@google.com>
> >> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> >> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >> Cc: Peter Zijlstra <peterz@infradead.org>
> >> Cc: Sasha Levin <sasha.levin@oracle.com>
> >> Cc: Thomas Gleixner <tglx@linutronix.de>
> >> Cc: Wolfram Gloger <wmglo@dent.med.uni-muenchen.de>
> >> Cc: kasan-dev <kasan-dev@googlegroups.com>
> >> Link: http://lkml.kernel.org/r/1444750088-24444-2-git-send-email-aryabinin@virtuozzo.com
> >> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> >> ---
> >>  include/linux/compiler-gcc.h | 13 ++++++++++
> >>  include/linux/compiler.h     | 60 ++++++++++++++++++++++++++++++++++----------
> >>  2 files changed, 60 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> >> index dfaa7b3..f2a9aec 100644
> >> --- a/include/linux/compiler-gcc.h
> >> +++ b/include/linux/compiler-gcc.h
> >> @@ -237,12 +237,25 @@
> >>  #define KASAN_ABI_VERSION 3
> >>  #endif
> >>
> >> +#if GCC_VERSION >= 40902
> >> +/*
> >> + * Tell the compiler that address safety instrumentation (KASAN)
> >> + * should not be applied to that function.
> >> + * Confilcts with inlining: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
> >> + */
> >> +#define __no_sanitize_address __attribute__((no_sanitize_address))
> >> +#endif
> >> +
> >>  #endif       /* gcc version >= 40000 specific checks */
> >>
> >>  #if !defined(__noclone)
> >>  #define __noclone    /* not needed */
> >>  #endif
> >>
> >> +#if !defined(__no_sanitize_address)
> >> +#define __no_sanitize_address
> >> +#endif
> >> +
> >>  /*
> >>   * A trick to suppress uninitialized variable warning without generating any
> >>   * code
> >> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> >> index c836eb2..aa2ae4c 100644
> >> --- a/include/linux/compiler.h
> >> +++ b/include/linux/compiler.h
> >> @@ -198,19 +198,42 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
> >>
> >>  #include <uapi/linux/types.h>
> >>
> >> -static __always_inline void __read_once_size(const volatile void *p, void *res, int size)
> >> +#define __READ_ONCE_SIZE                                             \
> >> +({                                                                   \
> >> +     switch (size) {                                                 \
> >> +     case 1: *(__u8 *)res = *(volatile __u8 *)p; break;              \
> >> +     case 2: *(__u16 *)res = *(volatile __u16 *)p; break;            \
> >> +     case 4: *(__u32 *)res = *(volatile __u32 *)p; break;            \
> >> +     case 8: *(__u64 *)res = *(volatile __u64 *)p; break;            \
> >> +     default:                                                        \
> >> +             barrier();                                              \
> >> +             __builtin_memcpy((void *)res, (const void *)p, size);   \
> >> +             barrier();                                              \
> >> +     }                                                               \
> >> +})
> >> +
> >> +static __always_inline
> >> +void __read_once_size_check(const volatile void *p, void *res, int size)
> >>  {
> >> -     switch (size) {
> >> -     case 1: *(__u8 *)res = *(volatile __u8 *)p; break;
> >> -     case 2: *(__u16 *)res = *(volatile __u16 *)p; break;
> >> -     case 4: *(__u32 *)res = *(volatile __u32 *)p; break;
> >> -     case 8: *(__u64 *)res = *(volatile __u64 *)p; break;
> >> -     default:
> >> -             barrier();
> >> -             __builtin_memcpy((void *)res, (const void *)p, size);
> >> -             barrier();
> >> -     }
> >> +     __READ_ONCE_SIZE;
> >> +}
> >> +
> >> +#ifdef CONFIG_KASAN
> >> +/*
> >> + * This function is not 'inline' because __no_sanitize_address confilcts
> >> + * with inlining. Attempt to inline it may cause a build failure.
> >> + *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
> >> + * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
> >> + */
> >> +static __no_sanitize_address __maybe_unused
> >> +void __read_once_size_nocheck(const volatile void *p, void *res, int size)
> >> +{
> >> +     __READ_ONCE_SIZE;
> >>  }
> >> +#else
> >> +static __always_inline __alias(__read_once_size_check)
> >> +void __read_once_size_nocheck(const volatile void *p, void *res, int size);
> >> +#endif
> >>
> >>  static __always_inline void __write_once_size(volatile void *p, void *res, int size)
> >>  {
> >> @@ -248,8 +271,19 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
> >>   * required ordering.
> >>   */
> >>
> >> -#define READ_ONCE(x) \
> >> -     ({ union { typeof(x) __val; char __c[1]; } __u; __read_once_size(&(x), __u.__c, sizeof(x)); __u.__val; })
> >> +#define __READ_ONCE(x, check)                                        \
> >> +({                                                           \
> >> +     union { typeof(x) __val; char __c[1]; } __u;            \
> >> +     __read_once_size##check(&(x), __u.__c, sizeof(x));      \
> >> +     __u.__val;                                              \
> >> +})
> >> +#define READ_ONCE(x) __READ_ONCE(x, _check)
> >> +
> >> +/*
> >> + * Use READ_ONCE_NOCHECK() instead of READ_ONCE() if you need
> >> + * to hide memory access from KASAN.
> >> + */
> >> +#define READ_ONCE_NOCHECK(x) __READ_ONCE(x, _nocheck)
> >>
> >>  #define WRITE_ONCE(x, val) \
> >>  ({                                                   \
> >>
> >
> > --
> > You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> > To post to this group, send email to kasan-dev@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20151014154532.GV3910%40linux.vnet.ibm.com.
> > For more options, visit https://groups.google.com/d/optout.
>