Re: [PATCH v5 00/18] Rework READ_ONCE() to improve codegen

[Marco Elver]

On Wed, 13 May 2020 at 18:50, Will Deacon <will@xxxxxxxxxx> wrote:
>
> On Wed, May 13, 2020 at 03:15:55PM +0200, Marco Elver wrote:
> > On Wed, 13 May 2020 at 14:40, Will Deacon <will@xxxxxxxxxx> wrote:
> > >
> > > On Wed, May 13, 2020 at 02:32:43PM +0200, Peter Zijlstra wrote:
> > > > On Wed, May 13, 2020 at 01:48:41PM +0200, Marco Elver wrote:
> > > >
> > > > > Disabling most instrumentation for arch/x86 is reasonable. Also fine
> > > > > with the __READ_ONCE/__WRITE_ONCE changes (your improved
> > > > > compiler-friendlier version).
> > > > >
> > > > > We likely can't have both: still instrument __READ_ONCE/__WRITE_ONCE
> > > > > (as Will suggested) *and* avoid double-instrumentation in arch_atomic.
> > > > > If most use-cases of __READ_ONCE/__WRITE_ONCE are likely to use
> > > > > data_race() or KCSAN_SANITIZE := n anyway, I'd say it's reasonable for
> > > > > now.
> > >
> > > I agree that Peter's patch is the right thing to do for now. I was hoping we
> > > could instrument __{READ,WRITE}_ONCE(), but that we before I realised that
> > > __no_sanitize_or_inline doesn't seem to do anything.
> > >
> > > > Right, if/when people want sanitize crud enabled for x86 I need
> > > > something that:
> > > >
> > > >  - can mark a function 'no_sanitize' and all code that gets inlined into
> > > >    that function must automagically also not get sanitized. ie. make
> > > >    inline work like macros (again).
> > > >
> > > > And optionally:
> > > >
> > > >  - can mark a function explicitly 'sanitize', and only when an explicit
> > > >    sanitize and no_sanitize mix in inlining give the current
> > > >    incompatible attribute splat.
> > > >
> > > > That way we can have the noinstr function attribute imply no_sanitize
> > > > and frob the DEFINE_IDTENTRY*() macros to use (a new) sanitize_or_inline
> > > > helper instead of __always_inline for __##func().
> > >
> > > Sounds like a good plan to me, assuming the compiler folks are onboard.
> > > In the meantime, can we kill __no_sanitize_or_inline and put it back to
> > > the old __no_kasan_or_inline, which I think simplifies compiler.h and
> > > doesn't mislead people into using the function annotation to avoid KCSAN?
> > >
> > > READ_ONCE_NOCHECK should also probably be READ_ONCE_NOKASAN, but I
> > > appreciate that's a noisier change.
> >
> > So far so good, except: both __no_sanitize_or_inline and
> > __no_kcsan_or_inline *do* avoid KCSAN instrumenting plain accesses, it
> > just doesn't avoid explicit kcsan_check calls, like those in
> > READ/WRITE_ONCE if KCSAN is enabled for the compilation unit. That's
> > just because macros won't be redefined just for __no_sanitize
> > functions. Similarly, READ_ONCE_NOCHECK does work as expected, and its
> > access is unchecked.
> >
> > This will have the expected result:
> > __no_sanitize_or_inline void foo(void) { x++; } // no data races reported
> >
> > This will not work as expected:
> > __no_sanitize_or_inline void foo(void) { READ_ONCE(x); }  // data
> > races are reported
>
> But the problem is that *this* does not work as expected:
>
> unsigned long __no_sanitize_or_inline foo(unsigned long *ptr)
> {
>         return READ_ONCE_NOCHECK(*ptr);
> }
>
> which I think means that the function annotation is practically useless.

Let me understand the problem better:

- We do not want __tsan_func_entry/exit (looking at the disassembly,
these aren't always generated).
- We do not want kcsan_disable/enable calls (with the new __READ_ONCE version).
- We do *not* want the call to __read_once_word_nocheck if we have
__no_sanitize_or_inline. AFAIK that's the main problem -- this applies
to both KASAN and KCSAN.

>From what I gather, we want to just compile the function as if the
sanitizer was never enabled. One reason for why this doesn't quite
work is because of the preprocessor.

Note that the sanitizers won't complain about these accesses, which
unfortunately is all these attributes ever were documented to do. So
the attributes aren't completely useless. Why doesn't
K[AC]SAN_SANITIZE := n work?

Thanks,
-- Marco