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

[Will Deacon]

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?

Will