Re: [PATCH] kcsan, compiler_types: Introduce __data_racy type qualifier

[Paul E. McKenney]

On Thu, May 02, 2024 at 04:12:17PM +0200, Marco Elver wrote:
> Based on the discussion at [1], it would be helpful to mark certain
> variables as explicitly "data racy", which would result in KCSAN not
> reporting data races involving any accesses on such variables. To do
> that, introduce the __data_racy type qualifier:
> 
> 	struct foo {
> 		...
> 		int __data_racy bar;
> 		...
> 	};
> 
> In KCSAN-kernels, __data_racy turns into volatile, which KCSAN already
> treats specially by considering them "marked". In non-KCSAN kernels the
> type qualifier turns into no-op.
> 
> The generated code between KCSAN-instrumented kernels and non-KCSAN
> kernels is already huge (inserted calls into runtime for every memory
> access), so the extra generated code (if any) due to volatile for few
> such __data_racy variables are unlikely to have measurable impact on
> performance.
> 
> Link: https://lore.kernel.org/all/CAHk-=wi3iondeh_9V2g3Qz5oHTRjLsOpoy83hb58MVh=nRZe0A@xxxxxxxxxxxxxx/ [1]
> Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Marco Elver <elver@xxxxxxxxxx>
> Cc: Paul E. McKenney <paulmck@xxxxxxxxxx>
> Cc: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>

I have queued and pushed this, thank you!

I have started testing, and if all goes well I will rebase this on top
of v6.9-rc2 (same base as the rest of my commits for next merge window),
merge it in and push it out.  With a little luck, this will get it into
tomorrow's -next.  With more luck than anyone deserves, today's.

							Thanx, Paul

> ---
>  Documentation/dev-tools/kcsan.rst | 10 ++++++++++
>  include/linux/compiler_types.h    |  7 +++++++
>  kernel/kcsan/kcsan_test.c         | 17 +++++++++++++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/Documentation/dev-tools/kcsan.rst b/Documentation/dev-tools/kcsan.rst
> index 94b6802ab0ab..02143f060b22 100644
> --- a/Documentation/dev-tools/kcsan.rst
> +++ b/Documentation/dev-tools/kcsan.rst
> @@ -91,6 +91,16 @@ the below options are available:
>    behaviour when encountering a data race is deemed safe.  Please see
>    `"Marking Shared-Memory Accesses" in the LKMM`_ for more information.
>  
> +* Similar to ``data_race(...)``, the type qualifier ``__data_racy`` can be used
> +  to document that all data races due to accesses to a variable are intended
> +  and should be ignored by KCSAN::
> +
> +    struct foo {
> +        ...
> +        int __data_racy stats_counter;
> +        ...
> +    };
> +
>  * Disabling data race detection for entire functions can be accomplished by
>    using the function attribute ``__no_kcsan``::
>  
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 2abaa3a825a9..a38162a8590d 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -273,9 +273,16 @@ struct ftrace_likely_data {
>   * disable all instrumentation. See Kconfig.kcsan where this is mandatory.
>   */
>  # define __no_kcsan __no_sanitize_thread __disable_sanitizer_instrumentation
> +/*
> + * Type qualifier to mark variables where all data-racy accesses should be
> + * ignored by KCSAN. Note, the implementation simply marks these variables as
> + * volatile, since KCSAN will treat such accesses as "marked".
> + */
> +# define __data_racy volatile
>  # define __no_sanitize_or_inline __no_kcsan notrace __maybe_unused
>  #else
>  # define __no_kcsan
> +# define __data_racy
>  #endif
>  
>  #ifndef __no_sanitize_or_inline
> diff --git a/kernel/kcsan/kcsan_test.c b/kernel/kcsan/kcsan_test.c
> index 015586217875..0c17b4c83e1c 100644
> --- a/kernel/kcsan/kcsan_test.c
> +++ b/kernel/kcsan/kcsan_test.c
> @@ -304,6 +304,7 @@ static long test_array[3 * PAGE_SIZE / sizeof(long)];
>  static struct {
>  	long val[8];
>  } test_struct;
> +static long __data_racy test_data_racy;
>  static DEFINE_SEQLOCK(test_seqlock);
>  static DEFINE_SPINLOCK(test_spinlock);
>  static DEFINE_MUTEX(test_mutex);
> @@ -358,6 +359,8 @@ static noinline void test_kernel_write_uninstrumented(void) { test_var++; }
>  
>  static noinline void test_kernel_data_race(void) { data_race(test_var++); }
>  
> +static noinline void test_kernel_data_racy_qualifier(void) { test_data_racy++; }
> +
>  static noinline void test_kernel_assert_writer(void)
>  {
>  	ASSERT_EXCLUSIVE_WRITER(test_var);
> @@ -1009,6 +1012,19 @@ static void test_data_race(struct kunit *test)
>  	KUNIT_EXPECT_FALSE(test, match_never);
>  }
>  
> +/* Test the __data_racy type qualifier. */
> +__no_kcsan
> +static void test_data_racy_qualifier(struct kunit *test)
> +{
> +	bool match_never = false;
> +
> +	begin_test_checks(test_kernel_data_racy_qualifier, test_kernel_data_racy_qualifier);
> +	do {
> +		match_never = report_available();
> +	} while (!end_test_checks(match_never));
> +	KUNIT_EXPECT_FALSE(test, match_never);
> +}
> +
>  __no_kcsan
>  static void test_assert_exclusive_writer(struct kunit *test)
>  {
> @@ -1424,6 +1440,7 @@ static struct kunit_case kcsan_test_cases[] = {
>  	KCSAN_KUNIT_CASE(test_read_plain_atomic_rmw),
>  	KCSAN_KUNIT_CASE(test_zero_size_access),
>  	KCSAN_KUNIT_CASE(test_data_race),
> +	KCSAN_KUNIT_CASE(test_data_racy_qualifier),
>  	KCSAN_KUNIT_CASE(test_assert_exclusive_writer),
>  	KCSAN_KUNIT_CASE(test_assert_exclusive_access),
>  	KCSAN_KUNIT_CASE(test_assert_exclusive_access_writer),
> -- 
> 2.45.0.rc1.225.g2a3ae87e7f-goog
>