Re: [PATCH -tip v2 03/11] kcsan: Support distinguishing volatile accesses

[Marco Elver]

On Thu, 21 May 2020 at 15:18, Will Deacon <will@xxxxxxxxxx> wrote:
>
> On Thu, May 21, 2020 at 01:08:46PM +0200, Marco Elver wrote:
> > In the kernel, volatile is used in various concurrent context, whether
> > in low-level synchronization primitives or for legacy reasons. If
> > supported by the compiler, we will assume that aligned volatile accesses
> > up to sizeof(long long) (matching compiletime_assert_rwonce_type()) are
> > atomic.
> >
> > Recent versions Clang [1] (GCC tentative [2]) can instrument volatile
> > accesses differently. Add the option (required) to enable the
> > instrumentation, and provide the necessary runtime functions. None of
> > the updated compilers are widely available yet (Clang 11 will be the
> > first release to support the feature).
> >
> > [1] https://github.com/llvm/llvm-project/commit/5a2c31116f412c3b6888be361137efd705e05814
> > [2] https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544452.html
> >
> > This patch allows removing any explicit checks in primitives such as
> > READ_ONCE() and WRITE_ONCE().
> >
> > Signed-off-by: Marco Elver <elver@xxxxxxxxxx>
> > ---
> > v2:
> > * Reword Makefile comment.
> > ---
> >  kernel/kcsan/core.c    | 43 ++++++++++++++++++++++++++++++++++++++++++
> >  scripts/Makefile.kcsan |  5 ++++-
> >  2 files changed, 47 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c
> > index a73a66cf79df..15f67949d11e 100644
> > --- a/kernel/kcsan/core.c
> > +++ b/kernel/kcsan/core.c
> > @@ -789,6 +789,49 @@ void __tsan_write_range(void *ptr, size_t size)
> >  }
> >  EXPORT_SYMBOL(__tsan_write_range);
> >
> > +/*
> > + * Use of explicit volatile is generally disallowed [1], however, volatile is
> > + * still used in various concurrent context, whether in low-level
> > + * synchronization primitives or for legacy reasons.
> > + * [1] https://lwn.net/Articles/233479/
> > + *
> > + * We only consider volatile accesses atomic if they are aligned and would pass
> > + * the size-check of compiletime_assert_rwonce_type().
> > + */
> > +#define DEFINE_TSAN_VOLATILE_READ_WRITE(size)                                  \
> > +     void __tsan_volatile_read##size(void *ptr)                             \
> > +     {                                                                      \
> > +             const bool is_atomic = size <= sizeof(long long) &&            \
> > +                                    IS_ALIGNED((unsigned long)ptr, size);   \
> > +             if (IS_ENABLED(CONFIG_KCSAN_IGNORE_ATOMICS) && is_atomic)      \
> > +                     return;                                                \
> > +             check_access(ptr, size, is_atomic ? KCSAN_ACCESS_ATOMIC : 0);  \
> > +     }                                                                      \
> > +     EXPORT_SYMBOL(__tsan_volatile_read##size);                             \
> > +     void __tsan_unaligned_volatile_read##size(void *ptr)                   \
> > +             __alias(__tsan_volatile_read##size);                           \
> > +     EXPORT_SYMBOL(__tsan_unaligned_volatile_read##size);                   \
> > +     void __tsan_volatile_write##size(void *ptr)                            \
> > +     {                                                                      \
> > +             const bool is_atomic = size <= sizeof(long long) &&            \
> > +                                    IS_ALIGNED((unsigned long)ptr, size);   \
> > +             if (IS_ENABLED(CONFIG_KCSAN_IGNORE_ATOMICS) && is_atomic)      \
> > +                     return;                                                \
> > +             check_access(ptr, size,                                        \
> > +                          KCSAN_ACCESS_WRITE |                              \
> > +                                  (is_atomic ? KCSAN_ACCESS_ATOMIC : 0));   \
> > +     }                                                                      \
> > +     EXPORT_SYMBOL(__tsan_volatile_write##size);                            \
> > +     void __tsan_unaligned_volatile_write##size(void *ptr)                  \
> > +             __alias(__tsan_volatile_write##size);                          \
> > +     EXPORT_SYMBOL(__tsan_unaligned_volatile_write##size)
> > +
> > +DEFINE_TSAN_VOLATILE_READ_WRITE(1);
> > +DEFINE_TSAN_VOLATILE_READ_WRITE(2);
> > +DEFINE_TSAN_VOLATILE_READ_WRITE(4);
> > +DEFINE_TSAN_VOLATILE_READ_WRITE(8);
> > +DEFINE_TSAN_VOLATILE_READ_WRITE(16);
>
> Having a 16-byte case seems a bit weird to me, but I guess clang needs this
> for some reason?

Yes, the emitted fixed-size instrumentation is up to 16 bytes, so
we'll need it (for both volatile and non-volatile -- otherwise we'll
get linker errors). It doesn't mean we'll consider 16 byte volatile
accesses as atomic, because of the size check to compute is_atomic
above.

Thanks,
-- Marco