Re: [PATCH] READ_ONCE, WRITE_ONCE, kcsan: Perform checks in __*_ONCE variants

[Marco Elver]

On Tue, 19 May 2020 at 23:10, Qian Cai <cai@xxxxxx> wrote:
>
> On Tue, May 12, 2020 at 3:09 PM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> >
> > On Tue, May 12, 2020 at 08:38:39PM +0200, Marco Elver wrote:
> > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > > index 741c93c62ecf..e902ca5de811 100644
> > > --- a/include/linux/compiler.h
> > > +++ b/include/linux/compiler.h
> > > @@ -224,13 +224,16 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
> > >   * atomicity or dependency ordering guarantees. Note that this may result
> > >   * in tears!
> > >   */
> > > -#define __READ_ONCE(x)       (*(const volatile __unqual_scalar_typeof(x) *)&(x))
> > > +#define __READ_ONCE(x)                                                       \
> > > +({                                                                   \
> > > +     kcsan_check_atomic_read(&(x), sizeof(x));                       \
> > > +     data_race((*(const volatile __unqual_scalar_typeof(x) *)&(x))); \
> > > +})
> >
> > NAK
> >
> > This will actively insert instrumentation into __READ_ONCE() and I need
> > it to not have any.
>
> Any way to move this forward? Due to linux-next commit 6bcc8f459fe7
> (locking/atomics: Flip fallbacks and instrumentation), it triggers a
> lots of KCSAN warnings due to atomic ops are no longer marked.

This is no longer the right solution we believe due to the various
requirements that Peter also mentioned. See the discussion here:
    https://lkml.kernel.org/r/CANpmjNOGFqhtDa9wWpXs2kztQsSozbwsuMO5BqqW0c0g0zGfSA@xxxxxxxxxxxxxx

The new solution is here:
    https://lkml.kernel.org/r/20200515150338.190344-1-elver@xxxxxxxxxx
While it's a little inconvenient that we'll require Clang 11
(currently available by building yourself from LLVM repo), but until
we get GCC fixed (my patch there still pending :-/), this is probably
the right solution going forward.   If possible, please do test!

Thanks,
-- Marco

> For
> example,
> [  197.318288][ T1041] write to 0xffff9302764ccc78 of 8 bytes by task
> 1048 on cpu 47:
> [  197.353119][ T1041]  down_read_trylock+0x9e/0x1e0
> atomic_long_set(&sem->owner, val);
> __rwsem_set_reader_owned at kernel/locking/rwsem.c:205
> (inlined by) rwsem_set_reader_owned at kernel/locking/rwsem.c:213
> (inlined by) __down_read_trylock at kernel/locking/rwsem.c:1373
> (inlined by) down_read_trylock at kernel/locking/rwsem.c:1517
> [  197.374641][ T1041]  page_lock_anon_vma_read+0x19d/0x3c0
> [  197.398894][ T1041]  rmap_walk_anon+0x30e/0x620
>
> [  197.924695][ T1041] read to 0xffff9302764ccc78 of 8 bytes by task
> 1041 on cpu 43:
> [  197.959501][ T1041]  up_read+0xb8/0x41a
> arch_atomic64_read at arch/x86/include/asm/atomic64_64.h:22
> (inlined by) atomic64_read at include/asm-generic/atomic-instrumented.h:838
> (inlined by) atomic_long_read at include/asm-generic/atomic-long.h:29
> (inlined by) rwsem_clear_reader_owned at kernel/locking/rwsem.c:242
> (inlined by) __up_read at kernel/locking/rwsem.c:1433
> (inlined by) up_read at kernel/locking/rwsem.c:1574
> [  197.977728][ T1041]  rmap_walk_anon+0x2f2/0x620
> [  197.999055][ T1041]  rmap_walk+0xb5/0xe0