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

Will Deacon
2020-05-21T13:18:12Z

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@google.com>
> ---
> 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?

Will