[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <f140eb01-fc94-478b-8931-3e1d281949ce@paulmck-laptop>
Date: Tue, 7 May 2024 09:56:45 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Marco Elver <elver@...gle.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
Mark Rutland <mark.rutland@....com>,
Dmitry Vyukov <dvyukov@...gle.com>,
Alexander Potapenko <glider@...gle.com>,
Boqun Feng <boqun.feng@...il.com>, kasan-dev@...glegroups.com,
linux-kernel@...r.kernel.org,
Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Subject: Re: [PATCH] kcsan, compiler_types: Introduce __data_racy type
qualifier
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@mail.gmail.com/ [1]
> Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Signed-off-by: Marco Elver <elver@...gle.com>
> Cc: Paul E. McKenney <paulmck@...nel.org>
> Cc: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
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
>
Powered by blists - more mailing lists