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

From: Paul E. McKenney
Date: Tue May 07 2024 - 12:57:16 EST


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
>