From: Marco Elver <elver@google.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>,
Dmitry Vyukov <dvyukov@google.com>,
Alexander Potapenko <glider@google.com>,
Andrey Konovalov <andreyknvl@google.com>,
kasan-dev <kasan-dev@googlegroups.com>,
LKML <linux-kernel@vger.kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@kernel.org>, Will Deacon <will@kernel.org>,
clang-built-linux <clang-built-linux@googlegroups.com>,
Borislav Petkov <bp@alien8.de>
Subject: Re: [PATCH -tip v3 05/11] kcsan: Remove 'noinline' from __no_kcsan_or_inline
Date: Fri, 29 May 2020 20:36:56 +0200 [thread overview]
Message-ID: <CANpmjNPaL=HRvaJOC37_Cf4S4kskZezmgRiDSGn460rO2dM4+g@mail.gmail.com> (raw)
In-Reply-To: <20200529170755.GN706495@hirez.programming.kicks-ass.net>
On Fri, 29 May 2020 at 19:08, Peter Zijlstra <peterz@infradead.org> wrote:
[...]
>
> Doesn't this mean we can do the below?
If nobody complains about the lack of __no_kcsan_or_inline, let's do
it. See comments below.
> ---
> Documentation/dev-tools/kcsan.rst | 6 ------
> arch/x86/include/asm/bitops.h | 6 +-----
> include/linux/compiler_types.h | 14 ++++----------
> kernel/kcsan/kcsan-test.c | 4 ++--
> 4 files changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/Documentation/dev-tools/kcsan.rst b/Documentation/dev-tools/kcsan.rst
> index ce4bbd918648..b38379f06194 100644
> --- a/Documentation/dev-tools/kcsan.rst
> +++ b/Documentation/dev-tools/kcsan.rst
> @@ -114,12 +114,6 @@ functions, compilation units, or entire subsystems. For static blacklisting,
> To dynamically limit for which functions to generate reports, see the
> `DebugFS interface`_ blacklist/whitelist feature.
>
> - For ``__always_inline`` functions, replace ``__always_inline`` with
> - ``__no_kcsan_or_inline`` (which implies ``__always_inline``)::
> -
> - static __no_kcsan_or_inline void foo(void) {
> - ...
> -
> * To disable data race detection for a particular compilation unit, add to the
> ``Makefile``::
I suppose, if we say that __no_kcsan_or_inline should just disappear
because '__no_kcsan inline' is now good enough, we can delete it.
I think functions that absolutely must be __always_inline would break
with __no_kcsan_or_inline under KCSAN anyway. So, let's simplify.
> diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
> index 35460fef39b8..0367efdc5b7a 100644
> --- a/arch/x86/include/asm/bitops.h
> +++ b/arch/x86/include/asm/bitops.h
> @@ -201,12 +201,8 @@ arch_test_and_change_bit(long nr, volatile unsigned long *addr)
> return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btc), *addr, c, "Ir", nr);
> }
>
> -static __no_kcsan_or_inline bool constant_test_bit(long nr, const volatile unsigned long *addr)
> +static __always_inline bool constant_test_bit(long nr, const volatile unsigned long *addr)
> {
> - /*
> - * Because this is a plain access, we need to disable KCSAN here to
> - * avoid double instrumentation via instrumented bitops.
> - */
Yes, we should have reverted this eventually.
> return ((1UL << (nr & (BITS_PER_LONG-1))) &
> (addr[nr >> _BITOPS_LONG_SHIFT])) != 0;
> }
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 4e4982d6f3b0..6a2c0f857ac3 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -118,10 +118,6 @@ struct ftrace_likely_data {
> #define notrace __attribute__((__no_instrument_function__))
> #endif
>
> -/* Section for code which can't be instrumented at all */
> -#define noinstr \
> - noinline notrace __attribute((__section__(".noinstr.text")))
> -
> /*
> * it doesn't make sense on ARM (currently the only user of __naked)
> * to trace naked functions because then mcount is called without
> @@ -192,17 +188,15 @@ struct ftrace_likely_data {
> #endif
>
> #define __no_kcsan __no_sanitize_thread
> -#ifdef __SANITIZE_THREAD__
> -# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused
> -# define __no_sanitize_or_inline __no_kcsan_or_inline
I think we just want to keep __no_sanitize_or_inline, for
READ_ONCE_NOCHECK. Having READ_ONCE_NOCHECK do KCSAN-checking seems
wrong, and I don't know what might break.
> -#else
> -# define __no_kcsan_or_inline __always_inline
> -#endif
>
> #ifndef __no_sanitize_or_inline
> #define __no_sanitize_or_inline __always_inline
> #endif
>
> +/* Section for code which can't be instrumented at all */
> +#define noinstr \
> + noinline notrace __attribute((__section__(".noinstr.text"))) __no_kcsan
> +
Will this eventually need __no_sanitize_address?
> #endif /* __KERNEL__ */
>
> #endif /* __ASSEMBLY__ */
> diff --git a/kernel/kcsan/kcsan-test.c b/kernel/kcsan/kcsan-test.c
> index a8c11506dd2a..374263ddffe2 100644
> --- a/kernel/kcsan/kcsan-test.c
> +++ b/kernel/kcsan/kcsan-test.c
> @@ -43,7 +43,7 @@ static struct {
> };
>
> /* Setup test checking loop. */
> -static __no_kcsan_or_inline void
> +static __no_kcsan inline void
> begin_test_checks(void (*func1)(void), void (*func2)(void))
> {
> kcsan_disable_current();
> @@ -60,7 +60,7 @@ begin_test_checks(void (*func1)(void), void (*func2)(void))
> }
>
> /* End test checking loop. */
> -static __no_kcsan_or_inline bool
> +static __no_kcsan inline bool
> end_test_checks(bool stop)
> {
> if (!stop && time_before(jiffies, end_time)) {
Acked -- if you send a patch, do split the test-related change, so
that Paul can apply it to the test which is currently only in -rcu.
Thanks,
-- Marco
next prev parent reply other threads:[~2020-05-29 18:37 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-21 14:20 [PATCH -tip v3 00/11] Fix KCSAN for new ONCE (require Clang 11) Marco Elver
2020-05-21 14:20 ` [PATCH -tip v3 01/11] ubsan, kcsan: don't combine sanitizer with kcov on clang Marco Elver
2020-05-21 14:20 ` [PATCH -tip v3 02/11] kcsan: Avoid inserting __tsan_func_entry/exit if possible Marco Elver
2020-05-22 16:08 ` [tip: locking/kcsan] " tip-bot2 for Marco Elver
2020-05-21 14:20 ` [PATCH -tip v3 03/11] kcsan: Support distinguishing volatile accesses Marco Elver
2020-05-22 10:26 ` Borislav Petkov
2020-05-22 10:34 ` Marco Elver
2020-05-22 10:47 ` Borislav Petkov
2020-05-22 16:08 ` [tip: locking/kcsan] " tip-bot2 for Marco Elver
2020-05-21 14:20 ` [PATCH -tip v3 04/11] kcsan: Pass option tsan-instrument-read-before-write to Clang Marco Elver
2020-05-22 16:08 ` [tip: locking/kcsan] " tip-bot2 for Marco Elver
2020-05-21 14:20 ` [PATCH -tip v3 05/11] kcsan: Remove 'noinline' from __no_kcsan_or_inline Marco Elver
2020-05-29 17:07 ` Peter Zijlstra
2020-05-29 18:36 ` Marco Elver [this message]
2020-05-29 18:59 ` Peter Zijlstra
2020-05-21 14:20 ` [PATCH -tip v3 06/11] kcsan: Restrict supported compilers Marco Elver
2020-05-21 14:20 ` [PATCH -tip v3 07/11] kcsan: Update Documentation to change " Marco Elver
2020-05-22 16:08 ` [tip: locking/kcsan] " tip-bot2 for Marco Elver
2020-05-21 14:20 ` [PATCH -tip v3 08/11] READ_ONCE, WRITE_ONCE: Remove data_race() and unnecessary checks Marco Elver
2020-05-22 16:08 ` [tip: locking/kcsan] compiler.h: Remove data_race() and unnecessary checks from {READ,WRITE}_ONCE() tip-bot2 for Marco Elver
2020-05-21 14:20 ` [PATCH -tip v3 09/11] data_race: Avoid nested statement expression Marco Elver
2020-05-21 20:21 ` Nick Desaulniers
2020-05-26 10:42 ` Arnd Bergmann
2020-05-26 12:02 ` Will Deacon
2020-05-26 12:19 ` Arnd Bergmann
2020-05-26 13:12 ` Marco Elver
2020-05-26 17:33 ` Marco Elver
2020-05-26 19:00 ` Arnd Bergmann
2020-05-26 23:10 ` Arnd Bergmann
2020-05-27 7:22 ` Will Deacon
2020-05-27 7:44 ` Marco Elver
2020-05-27 9:26 ` Arnd Bergmann
2020-05-28 12:53 ` Stephen Rothwell
2020-05-26 21:36 ` Peter Zijlstra
2020-05-21 14:20 ` [PATCH -tip v3 10/11] compiler.h: Move function attributes to compiler_types.h Marco Elver
2020-05-22 16:08 ` [tip: locking/kcsan] " tip-bot2 for Marco Elver
2020-05-21 14:20 ` [PATCH -tip v3 11/11] compiler_types.h, kasan: Use __SANITIZE_ADDRESS__ instead of CONFIG_KASAN to decide inlining Marco Elver
2020-05-22 16:08 ` [tip: locking/kcsan] " tip-bot2 for Marco Elver
2020-05-22 11:35 ` [PATCH -tip v3 00/11] Fix KCSAN for new ONCE (require Clang 11) Peter Zijlstra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANpmjNPaL=HRvaJOC37_Cf4S4kskZezmgRiDSGn460rO2dM4+g@mail.gmail.com' \
--to=elver@google.com \
--cc=andreyknvl@google.com \
--cc=bp@alien8.de \
--cc=clang-built-linux@googlegroups.com \
--cc=dvyukov@google.com \
--cc=glider@google.com \
--cc=kasan-dev@googlegroups.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=paulmck@kernel.org \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).