[PATCH -tip 00/10] Fix KCSAN for new ONCE (require Clang 11)

[Marco Elver]

This patch series is the conclusion to [1], where we determined that due
to various interactions with no_sanitize attributes and the new
{READ,WRITE}_ONCE(), KCSAN will require Clang 11 or later. Other
sanitizers are largely untouched, and only KCSAN now has a hard
dependency on Clang 11. To test, a recent Clang development version will
suffice [2]. While a little inconvenient for now, it is hoped that in
future we may be able to fix GCC and re-enable GCC support.

The patch "kcsan: Restrict supported compilers" contains a detailed list
of requirements that led to this decision.

Most of the patches are related to KCSAN, however, the first patch also
includes an UBSAN related fix and is a dependency for the remaining
ones. The last 2 patches clean up the attributes by moving them to the
right place, and fix KASAN's way of defining __no_kasan_or_inline,
making it consistent with KCSAN.

The series has been tested by running kcsan-test several times and
completed successfully.

[1] https://lkml.kernel.org/r/CANpmjNOGFqhtDa9wWpXs2kzt...@mail.gmail.com
[2] https://github.com/llvm/llvm-project

Arnd Bergmann (1):
ubsan, kcsan: don't combine sanitizer with kcov on clang

Marco Elver (9):
kcsan: Avoid inserting __tsan_func_entry/exit if possible
kcsan: Support distinguishing volatile accesses
kcsan: Pass option tsan-instrument-read-before-write to Clang
kcsan: Remove 'noinline' from __no_kcsan_or_inline
kcsan: Restrict supported compilers
kcsan: Update Documentation to change supported compilers
READ_ONCE, WRITE_ONCE: Remove data_race() wrapping
compiler.h: Move function attributes to compiler_types.h
compiler_types.h, kasan: Use __SANITIZE_ADDRESS__ instead of
CONFIG_KASAN to decide inlining

Documentation/dev-tools/kcsan.rst | 9 +------
include/linux/compiler.h | 35 ++-----------------------
include/linux/compiler_types.h | 32 +++++++++++++++++++++++
kernel/kcsan/core.c | 43 +++++++++++++++++++++++++++++++
lib/Kconfig.kcsan | 20 +++++++++++++-
lib/Kconfig.ubsan | 11 ++++++++
scripts/Makefile.kcsan | 15 ++++++++++-
7 files changed, 122 insertions(+), 43 deletions(-)

--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

From: Arnd Bergmann <ar...@arndb.de>

Clang does not allow -fsanitize-coverage=trace-{pc,cmp} together
with -fsanitize=bounds or with ubsan:

clang: error: argument unused during compilation: '-fsanitize-coverage=trace-pc' [-Werror,-Wunused-command-line-argument]
clang: error: argument unused during compilation: '-fsanitize-coverage=trace-cmp' [-Werror,-Wunused-command-line-argument]

To avoid the warning, check whether clang can handle this correctly
or disallow ubsan and kcsan when kcov is enabled.

Link: https://bugs.llvm.org/show_bug.cgi?id=45831
Link: https://lore.kernel.org/lkml/20200505142341...@arndb.de
Acked-by: Marco Elver <el...@google.com>
Signed-off-by: Arnd Bergmann <ar...@arndb.de>
Signed-off-by: Marco Elver <el...@google.com>
---
This patch is already in -rcu tree, but since since the series is based
on -tip, to avoid conflict it is required for the subsequent patches.
---
lib/Kconfig.kcsan | 11 +++++++++++
lib/Kconfig.ubsan | 11 +++++++++++
2 files changed, 22 insertions(+)

diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index ea28245c6c1d..a7276035ca0d 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -3,9 +3,20 @@
config HAVE_ARCH_KCSAN
bool

+config KCSAN_KCOV_BROKEN
+ def_bool KCOV && CC_HAS_SANCOV_TRACE_PC
+ depends on CC_IS_CLANG
+ depends on !$(cc-option,-Werror=unused-command-line-argument -fsanitize=thread -fsanitize-coverage=trace-pc)
+ help
+ Some versions of clang support either KCSAN and KCOV but not the
+ combination of the two.
+ See https://bugs.llvm.org/show_bug.cgi?id=45831 for the status
+ in newer releases.
+
menuconfig KCSAN
bool "KCSAN: dynamic data race detector"
depends on HAVE_ARCH_KCSAN && DEBUG_KERNEL && !KASAN
+ depends on !KCSAN_KCOV_BROKEN
select STACKTRACE
help
The Kernel Concurrency Sanitizer (KCSAN) is a dynamic
diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
index 48469c95d78e..3baea77bf37f 100644
--- a/lib/Kconfig.ubsan
+++ b/lib/Kconfig.ubsan
@@ -26,9 +26,20 @@ config UBSAN_TRAP
the system. For some system builders this is an acceptable
trade-off.

+config UBSAN_KCOV_BROKEN
+ def_bool KCOV && CC_HAS_SANCOV_TRACE_PC
+ depends on CC_IS_CLANG
+ depends on !$(cc-option,-Werror=unused-command-line-argument -fsanitize=bounds -fsanitize-coverage=trace-pc)
+ help
+ Some versions of clang support either UBSAN or KCOV but not the
+ combination of the two.
+ See https://bugs.llvm.org/show_bug.cgi?id=45831 for the status
+ in newer releases.
+
config UBSAN_BOUNDS
bool "Perform array index bounds checking"
default UBSAN
+ depends on !UBSAN_KCOV_BROKEN
help
This option enables detection of directly indexed out of bounds
array accesses, where the array size is known at compile time.
--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

To avoid inserting __tsan_func_{entry,exit}, add option if supported by
compiler. Currently only Clang can be told to not emit calls to these
functions. It is safe to not emit these, since KCSAN does not rely on
them.

Note that, if we disable __tsan_func_{entry,exit}(), we need to disable
tail-call optimization in sanitized compilation units, as otherwise we
may skip frames in the stack trace; in particular when the tail called
function is one of the KCSAN's runtime functions, and a report is
generated, might we miss the function where the actual access occurred.
Since __tsan_func_{entry,exit}() insertion effectively disabled
tail-call optimization, there should be no observable change. [This was
caught and confirmed with kcsan-test & UNWINDER_ORC.]

Signed-off-by: Marco Elver <el...@google.com>
---

scripts/Makefile.kcsan | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/scripts/Makefile.kcsan b/scripts/Makefile.kcsan
index caf1111a28ae..20337a7ecf54 100644
--- a/scripts/Makefile.kcsan
+++ b/scripts/Makefile.kcsan
@@ -1,6 +1,15 @@
# SPDX-License-Identifier: GPL-2.0
ifdef CONFIG_KCSAN

-CFLAGS_KCSAN := -fsanitize=thread
+# GCC and Clang accept backend options differently. Do not wrap in cc-option,
+# because Clang accepts "--param" even if it is unused.
+ifdef CONFIG_CC_IS_CLANG
+cc-param = -mllvm -$(1)
+else
+cc-param = --param -$(1)
+endif
+
+CFLAGS_KCSAN := -fsanitize=thread \
+ $(call cc-option,$(call cc-param,tsan-instrument-func-entry-exit=0) -fno-optimize-sibling-calls)

endif # CONFIG_KCSAN
--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

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 <el...@google.com>
---

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);
+
/*
* The below are not required by KCSAN, but can still be emitted by the
* compiler.
diff --git a/scripts/Makefile.kcsan b/scripts/Makefile.kcsan
index 20337a7ecf54..c02662b30a7c 100644
--- a/scripts/Makefile.kcsan
+++ b/scripts/Makefile.kcsan
@@ -9,7 +9,10 @@ else
cc-param = --param -$(1)
endif

+# Most options here should be kept optional, to allow enabling more compilers
+# if the absence of some options still allows us to use KCSAN in most cases.
CFLAGS_KCSAN := -fsanitize=thread \
- $(call cc-option,$(call cc-param,tsan-instrument-func-entry-exit=0) -fno-optimize-sibling-calls)
+ $(call cc-option,$(call cc-param,tsan-instrument-func-entry-exit=0) -fno-optimize-sibling-calls) \
+ $(call cc-param,tsan-distinguish-volatile=1)

[Marco Elver]

Clang (unlike GCC) removes reads before writes with matching addresses
in the same basic block. This is an optimization for TSAN, since writes
will always cause conflict if the preceding read would have.

However, for KCSAN we cannot rely on this option, because we apply
several special rules to writes, in particular when the
KCSAN_ASSUME_PLAIN_WRITES_ATOMIC option is selected. To avoid missing
potential data races, pass the -tsan-instrument-read-before-write option
to Clang if it is available [1].

[1] https://github.com/llvm/llvm-project/commit/151ed6aa38a3ec6c01973b35f684586b6e1c0f7e

Signed-off-by: Marco Elver <el...@google.com>
---

scripts/Makefile.kcsan | 1 +
1 file changed, 1 insertion(+)

diff --git a/scripts/Makefile.kcsan b/scripts/Makefile.kcsan
index c02662b30a7c..ea4a6301633e 100644
--- a/scripts/Makefile.kcsan
+++ b/scripts/Makefile.kcsan
@@ -13,6 +13,7 @@ endif

# if the absence of some options still allows us to use KCSAN in most cases.
CFLAGS_KCSAN := -fsanitize=thread \

$(call cc-option,$(call cc-param,tsan-instrument-func-entry-exit=0) -fno-optimize-sibling-calls) \
+ $(call cc-option,$(call cc-param,tsan-instrument-read-before-write=1)) \

[Marco Elver]

Some compilers incorrectly inline small __no_kcsan functions, which then
results in instrumenting the accesses. For this reason, the 'noinline'
attribute was added to __no_kcsan_or_inline. All known versions of GCC
are affected by this. Supported version of Clang are unaffected, and
never inlines a no_sanitize function.

However, the attribute 'noinline' in __no_kcsan_or_inline causes
unexpected code generation in functions that are __no_kcsan and call a
__no_kcsan_or_inline function.

In certain situations it is expected that the __no_kcsan_or_inline
function is actually inlined by the __no_kcsan function, and *no* calls
are emitted. By removing the 'noinline' attribute we give the compiler
the ability to inline and generate the expected code in __no_kcsan
functions.

Link: https://lkml.kernel.org/r/CANpmjNNOpJk0tprXKB_deiNA...@mail.gmail.com

Signed-off-by: Marco Elver <el...@google.com>
---

include/linux/compiler.h | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index e24cc3a2bc3e..17c98b215572 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -276,11 +276,9 @@ do { \
#ifdef __SANITIZE_THREAD__
/*
* Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in
- * compilation units where instrumentation is disabled. The attribute 'noinline'
- * is required for older compilers, where implicit inlining of very small
- * functions renders __no_sanitize_thread ineffective.
+ * compilation units where instrumentation is disabled.
*/
-# define __no_kcsan_or_inline __no_kcsan noinline notrace __maybe_unused
+# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused
# define __no_sanitize_or_inline __no_kcsan_or_inline
#else
# define __no_kcsan_or_inline __always_inline
--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

The first version of Clang that supports -tsan-distinguish-volatile will
be able to support KCSAN. The first Clang release to do so, will be
Clang 11. This is due to satisfying all the following requirements:

1. Never emit calls to __tsan_func_{entry,exit}.

2. __no_kcsan functions should not call anything, not even
kcsan_{enable,disable}_current(), when using __{READ,WRITE}_ONCE => Requires
leaving them plain!

3. Support atomic_{read,set}*() with KCSAN, which rely on
arch_atomic_{read,set}*() using __{READ,WRITE}_ONCE() => Because of
#2, rely on Clang 11's -tsan-distinguish-volatile support. We will
double-instrument atomic_{read,set}*(), but that's reasonable given
it's still lower cost than the data_race() variant due to avoiding 2
extra calls (kcsan_{en,dis}able_current() calls).

4. __always_inline functions inlined into __no_kcsan functions are never
instrumented.

5. __always_inline functions inlined into instrumented functions are
instrumented.

6. __no_kcsan_or_inline functions may be inlined into __no_kcsan functions =>
Implies leaving 'noinline' off of __no_kcsan_or_inline.

7. Because of #6, __no_kcsan and __no_kcsan_or_inline functions should never be
spuriously inlined into instrumented functions, causing the accesses of the
__no_kcsan function to be instrumented.

Older versions of Clang do not satisfy #3. The latest GCC currently doesn't
support at least #1, #3, and #7.

Link: https://lkml.kernel.org/r/CANpmjNMTsY_8241bS7=XAfqvZHFLrVEkv_...@mail.gmail.com

Signed-off-by: Marco Elver <el...@google.com>
---

lib/Kconfig.kcsan | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index a7276035ca0d..3f3b5bca7a8f 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -3,6 +3,12 @@
config HAVE_ARCH_KCSAN
bool

+config HAVE_KCSAN_COMPILER
+ def_bool CC_IS_CLANG && $(cc-option,-fsanitize=thread -mllvm -tsan-distinguish-volatile=1)
+ help
+ For the list of compilers that support KCSAN, please see
+ <file:Documentation/dev-tools/kcsan.rst>.
+
config KCSAN_KCOV_BROKEN
def_bool KCOV && CC_HAS_SANCOV_TRACE_PC
depends on CC_IS_CLANG
@@ -15,7 +21,8 @@ config KCSAN_KCOV_BROKEN

menuconfig KCSAN
bool "KCSAN: dynamic data race detector"

- depends on HAVE_ARCH_KCSAN && DEBUG_KERNEL && !KASAN
+ depends on HAVE_ARCH_KCSAN && HAVE_KCSAN_COMPILER
+ depends on DEBUG_KERNEL && !KASAN

depends on !KCSAN_KCOV_BROKEN
select STACKTRACE
help

--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

Signed-off-by: Marco Elver <el...@google.com>
---

Documentation/dev-tools/kcsan.rst | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/Documentation/dev-tools/kcsan.rst b/Documentation/dev-tools/kcsan.rst
index f4b5766f12cc..ce4bbd918648 100644
--- a/Documentation/dev-tools/kcsan.rst
+++ b/Documentation/dev-tools/kcsan.rst
@@ -8,8 +8,7 @@ approach to detect races. KCSAN's primary purpose is to detect `data races`_.
Usage
-----

-KCSAN is supported in both GCC and Clang. With GCC it requires version 7.3.0 or
-later. With Clang it requires version 7.0.0 or later.
+KCSAN requires Clang version 11 or later.

To enable KCSAN configure the kernel with::

@@ -121,12 +120,6 @@ the below options are available:
static __no_kcsan_or_inline void foo(void) {
...

- Note: Older compiler versions (GCC < 9) also do not always honor the
- ``__no_kcsan`` attribute on regular ``inline`` functions. If false positives
- with these compilers cannot be tolerated, for small functions where
- ``__always_inline`` would be appropriate, ``__no_kcsan_or_inline`` should be
- preferred instead.
-
* To disable data race detection for a particular compilation unit, add to the
``Makefile``::

--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

The volatile access no longer needs to be wrapped in data_race(),
because we require compilers that emit instrumentation distinguishing
volatile accesses.

Signed-off-by: Marco Elver <el...@google.com>
---

include/linux/compiler.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 17c98b215572..fce56402c082 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -229,7 +229,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
#define __READ_ONCE_SCALAR(x) \
({ \
typeof(x) *__xp = &(x); \
- __unqual_scalar_typeof(x) __x = data_race(__READ_ONCE(*__xp)); \
+ __unqual_scalar_typeof(x) __x = __READ_ONCE(*__xp); \
kcsan_check_atomic_read(__xp, sizeof(*__xp)); \
smp_read_barrier_depends(); \
(typeof(x))__x; \
@@ -250,7 +250,7 @@ do { \
do { \
typeof(x) *__xp = &(x); \
kcsan_check_atomic_write(__xp, sizeof(*__xp)); \
- data_race(({ __WRITE_ONCE(*__xp, val); 0; })); \
+ __WRITE_ONCE(*__xp, val); \
} while (0)

#define WRITE_ONCE(x, val) \
--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

Cleanup and move the KASAN and KCSAN related function attributes to
compiler_types.h, where the rest of the same kind live.

No functional change intended.

Signed-off-by: Marco Elver <el...@google.com>
---

include/linux/compiler.h | 29 -----------------------------
include/linux/compiler_types.h | 29 +++++++++++++++++++++++++++++
2 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index fce56402c082..a7b01e750dd3 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -259,35 +259,6 @@ do { \
__WRITE_ONCE_SCALAR(x, val); \
} while (0)

-#ifdef CONFIG_KASAN
-/*
- * We can't declare function 'inline' because __no_sanitize_address conflicts
- * with inlining. Attempt to inline it may cause a build failure.
- * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
- * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
- */
-# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
-# define __no_sanitize_or_inline __no_kasan_or_inline
-#else
-# define __no_kasan_or_inline __always_inline
-#endif
-
-#define __no_kcsan __no_sanitize_thread
-#ifdef __SANITIZE_THREAD__
-/*
- * Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in

- * compilation units where instrumentation is disabled.

- */
-# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused
-# define __no_sanitize_or_inline __no_kcsan_or_inline
-#else
-# define __no_kcsan_or_inline __always_inline
-#endif
-
-#ifndef __no_sanitize_or_inline
-#define __no_sanitize_or_inline __always_inline
-#endif
-
static __no_sanitize_or_inline
unsigned long __read_once_word_nocheck(const void *addr)
{
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 6ed0612bc143..b190a12e7089 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -167,6 +167,35 @@ struct ftrace_likely_data {
*/
#define noinline_for_stack noinline

+#ifdef CONFIG_KASAN
+/*
+ * We can't declare function 'inline' because __no_sanitize_address conflicts
+ * with inlining. Attempt to inline it may cause a build failure.
+ * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
+ * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
+ */
+# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
+# define __no_sanitize_or_inline __no_kasan_or_inline
+#else
+# define __no_kasan_or_inline __always_inline
+#endif
+
+#define __no_kcsan __no_sanitize_thread
+#ifdef __SANITIZE_THREAD__
+/*
+ * Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in

+ * compilation units where instrumentation is disabled.

+ */

+# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused

+# define __no_sanitize_or_inline __no_kcsan_or_inline
+#else
+# define __no_kcsan_or_inline __always_inline
+#endif
+
+#ifndef __no_sanitize_or_inline
+#define __no_sanitize_or_inline __always_inline
+#endif
+
#endif /* __KERNEL__ */

#endif /* __ASSEMBLY__ */
--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

Like is done for KCSAN, for KASAN we should also use __always_inline in
compilation units that have instrumentation disabled
(KASAN_SANITIZE_foo.o := n). Adds common documentation for KASAN and
KCSAN explaining the attribute.

Signed-off-by: Marco Elver <el...@google.com>
---

include/linux/compiler_types.h | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index b190a12e7089..5faf68eae204 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -167,7 +167,14 @@ struct ftrace_likely_data {
*/
#define noinline_for_stack noinline

-#ifdef CONFIG_KASAN
+/*
+ * Sanitizer helper attributes: Because using __always_inline and
+ * __no_sanitize_* conflict, provide helper attributes that will either expand
+ * to __no_sanitize_* in compilation units where instrumentation is enabled
+ * (__SANITIZE_*__), or __always_inline in compilation units without
+ * instrumentation (__SANITIZE_*__ undefined).
+ */
+#ifdef __SANITIZE_ADDRESS__
/*

* We can't declare function 'inline' because __no_sanitize_address conflicts

* with inlining. Attempt to inline it may cause a build failure.

@@ -182,10 +189,6 @@ struct ftrace_likely_data {

#define __no_kcsan __no_sanitize_thread

#ifdef __SANITIZE_THREAD__
-/*
- * Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in
- * compilation units where instrumentation is disabled.
- */

# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused

# define __no_sanitize_or_inline __no_kcsan_or_inline
#else

--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

On Fri, 15 May 2020 at 17:04, Marco Elver <el...@google.com> wrote:
>
> The volatile access no longer needs to be wrapped in data_race(),
> because we require compilers that emit instrumentation distinguishing
> volatile accesses.
>
> Signed-off-by: Marco Elver <el...@google.com>
> ---
> include/linux/compiler.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 17c98b215572..fce56402c082 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -229,7 +229,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
> #define __READ_ONCE_SCALAR(x) \
> ({ \
> typeof(x) *__xp = &(x); \
> - __unqual_scalar_typeof(x) __x = data_race(__READ_ONCE(*__xp)); \
> + __unqual_scalar_typeof(x) __x = __READ_ONCE(*__xp); \
> kcsan_check_atomic_read(__xp, sizeof(*__xp)); \

Some self-review: We don't need kcsan_check_atomic anymore, and this
should be removed.

I'll send v2 to address this (together with fix to data_race()
removing nested statement expressions).

> smp_read_barrier_depends(); \
> (typeof(x))__x; \
> @@ -250,7 +250,7 @@ do { \
> do { \
> typeof(x) *__xp = &(x); \
> kcsan_check_atomic_write(__xp, sizeof(*__xp)); \

Same.

[Marco Elver]

On Thu, 21 May 2020 at 11:47, Marco Elver <el...@google.com> wrote:
>
> On Fri, 15 May 2020 at 17:04, Marco Elver <el...@google.com> wrote:
> >
> > The volatile access no longer needs to be wrapped in data_race(),
> > because we require compilers that emit instrumentation distinguishing
> > volatile accesses.
> >
> > Signed-off-by: Marco Elver <el...@google.com>
> > ---
> > include/linux/compiler.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > index 17c98b215572..fce56402c082 100644
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -229,7 +229,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
> > #define __READ_ONCE_SCALAR(x) \
> > ({ \
> > typeof(x) *__xp = &(x); \
> > - __unqual_scalar_typeof(x) __x = data_race(__READ_ONCE(*__xp)); \
> > + __unqual_scalar_typeof(x) __x = __READ_ONCE(*__xp); \
> > kcsan_check_atomic_read(__xp, sizeof(*__xp)); \
>
> Some self-review: We don't need kcsan_check_atomic anymore, and this
> should be removed.
>
> I'll send v2 to address this (together with fix to data_race()
> removing nested statement expressions).

The other thing here is that we no longer require __xp, and can just
pass x into __READ_ONCE.

> > smp_read_barrier_depends(); \
> > (typeof(x))__x; \
> > @@ -250,7 +250,7 @@ do { \
> > do { \
> > typeof(x) *__xp = &(x); \
> > kcsan_check_atomic_write(__xp, sizeof(*__xp)); \
>
> Same.

__xp can also be removed.

Note that this effectively aliases __WRITE_ONCE_SCALAR to
__WRITE_ONCE. To keep the API consistent with READ_ONCE, I assume we
want to keep __WRITE_ONCE_SCALAR, in case it is meant to change in
future?

[Will Deacon]

Ha! So I think this ends up being very similar to what I had *before* I
rebased onto KCSAN:

https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/tree/include/linux/compiler.h?h=rwonce/cleanup#n202

in which case you can drop __WRITE_ONCE_SCALAR; the _SCALAR things shouldn't
be used outside of the implementation anyway.

Will

[Marco Elver]

This patch series is the conclusion to [1], where we determined that due
to various interactions with no_sanitize attributes and the new
{READ,WRITE}_ONCE(), KCSAN will require Clang 11 or later. Other
sanitizers are largely untouched, and only KCSAN now has a hard
dependency on Clang 11. To test, a recent Clang development version will
suffice [2]. While a little inconvenient for now, it is hoped that in
future we may be able to fix GCC and re-enable GCC support.

The patch "kcsan: Restrict supported compilers" contains a detailed list
of requirements that led to this decision.

Most of the patches are related to KCSAN, however, the first patch also
includes an UBSAN related fix and is a dependency for the remaining
ones. The last 2 patches clean up the attributes by moving them to the
right place, and fix KASAN's way of defining __no_kasan_or_inline,
making it consistent with KCSAN.

The series has been tested by running kcsan-test several times and
completed successfully.

[1] https://lkml.kernel.org/r/CANpmjNOGFqhtDa9wWpXs2kzt...@mail.gmail.com
[2] https://github.com/llvm/llvm-project

v2:
* Remove unnecessary kcsan_check_atomic in ONCE.
* Simplify __READ_ONCE_SCALAR and remove __WRITE_ONCE_SCALAR. This
effectively restores Will Deacon's pre-KCSAN version:
https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/tree/include/linux/compiler.h?h=rwonce/cleanup#n202
* Introduce patch making data_race() a single statement expression in
response to apparent issues that compilers are having with nested
statement expressions.

Arnd Bergmann (1):
ubsan, kcsan: don't combine sanitizer with kcov on clang

Marco Elver (10):

kcsan: Avoid inserting __tsan_func_entry/exit if possible
kcsan: Support distinguishing volatile accesses
kcsan: Pass option tsan-instrument-read-before-write to Clang
kcsan: Remove 'noinline' from __no_kcsan_or_inline
kcsan: Restrict supported compilers
kcsan: Update Documentation to change supported compilers

READ_ONCE, WRITE_ONCE: Remove data_race() and unnecessary checks
data_race: Avoid nested statement expression

compiler.h: Move function attributes to compiler_types.h
compiler_types.h, kasan: Use __SANITIZE_ADDRESS__ instead of
CONFIG_KASAN to decide inlining

Documentation/dev-tools/kcsan.rst | 9 +-----

include/linux/compiler.h | 53 ++++---------------------------
include/linux/compiler_types.h | 32 +++++++++++++++++++
kernel/kcsan/core.c | 43 +++++++++++++++++++++++++
lib/Kconfig.kcsan | 20 +++++++++++-
lib/Kconfig.ubsan | 11 +++++++
scripts/Makefile.kcsan | 15 ++++++++-
7 files changed, 126 insertions(+), 57 deletions(-)

--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

From: Arnd Bergmann <ar...@arndb.de>

Clang does not allow -fsanitize-coverage=trace-{pc,cmp} together
with -fsanitize=bounds or with ubsan:

clang: error: argument unused during compilation: '-fsanitize-coverage=trace-pc' [-Werror,-Wunused-command-line-argument]
clang: error: argument unused during compilation: '-fsanitize-coverage=trace-cmp' [-Werror,-Wunused-command-line-argument]

To avoid the warning, check whether clang can handle this correctly
or disallow ubsan and kcsan when kcov is enabled.

Link: https://bugs.llvm.org/show_bug.cgi?id=45831
Link: https://lore.kernel.org/lkml/20200505142341...@arndb.de
Acked-by: Marco Elver <el...@google.com>
Signed-off-by: Arnd Bergmann <ar...@arndb.de>

Signed-off-by: Marco Elver <el...@google.com>
---

This patch is already in -rcu tree, but since since the series is based
on -tip, to avoid conflict it is required for the subsequent patches.
---
lib/Kconfig.kcsan | 11 +++++++++++
lib/Kconfig.ubsan | 11 +++++++++++
2 files changed, 22 insertions(+)

diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index ea28245c6c1d..a7276035ca0d 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -3,9 +3,20 @@
config HAVE_ARCH_KCSAN
bool

+config KCSAN_KCOV_BROKEN
+ def_bool KCOV && CC_HAS_SANCOV_TRACE_PC
+ depends on CC_IS_CLANG
+ depends on !$(cc-option,-Werror=unused-command-line-argument -fsanitize=thread -fsanitize-coverage=trace-pc)
+ help
+ Some versions of clang support either KCSAN and KCOV but not the
+ combination of the two.
+ See https://bugs.llvm.org/show_bug.cgi?id=45831 for the status
+ in newer releases.
+

menuconfig KCSAN
bool "KCSAN: dynamic data race detector"

depends on HAVE_ARCH_KCSAN && DEBUG_KERNEL && !KASAN

[Marco Elver]

To avoid inserting __tsan_func_{entry,exit}, add option if supported by
compiler. Currently only Clang can be told to not emit calls to these
functions. It is safe to not emit these, since KCSAN does not rely on
them.

Note that, if we disable __tsan_func_{entry,exit}(), we need to disable
tail-call optimization in sanitized compilation units, as otherwise we
may skip frames in the stack trace; in particular when the tail called
function is one of the KCSAN's runtime functions, and a report is
generated, might we miss the function where the actual access occurred.
Since __tsan_func_{entry,exit}() insertion effectively disabled
tail-call optimization, there should be no observable change. [This was
caught and confirmed with kcsan-test & UNWINDER_ORC.]

Signed-off-by: Marco Elver <el...@google.com>
---

scripts/Makefile.kcsan | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/scripts/Makefile.kcsan b/scripts/Makefile.kcsan
index caf1111a28ae..20337a7ecf54 100644
--- a/scripts/Makefile.kcsan
+++ b/scripts/Makefile.kcsan

@@ -1,6 +1,15 @@
# SPDX-License-Identifier: GPL-2.0
ifdef CONFIG_KCSAN

-CFLAGS_KCSAN := -fsanitize=thread
+# GCC and Clang accept backend options differently. Do not wrap in cc-option,
+# because Clang accepts "--param" even if it is unused.
+ifdef CONFIG_CC_IS_CLANG
+cc-param = -mllvm -$(1)
+else
+cc-param = --param -$(1)
+endif
+
+CFLAGS_KCSAN := -fsanitize=thread \

+ $(call cc-option,$(call cc-param,tsan-instrument-func-entry-exit=0) -fno-optimize-sibling-calls)

[Marco Elver]

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 <el...@google.com>
---

v2:
* Reword Makefile comment.

diff --git a/scripts/Makefile.kcsan b/scripts/Makefile.kcsan
index 20337a7ecf54..75d2942b9437 100644
--- a/scripts/Makefile.kcsan
+++ b/scripts/Makefile.kcsan

@@ -9,7 +9,10 @@ else
cc-param = --param -$(1)
endif

+# Keep most options here optional, to allow enabling more compilers if absence
+# of some options does not break KCSAN nor causes false positive reports.

CFLAGS_KCSAN := -fsanitize=thread \
- $(call cc-option,$(call cc-param,tsan-instrument-func-entry-exit=0) -fno-optimize-sibling-calls)

+ $(call cc-option,$(call cc-param,tsan-instrument-func-entry-exit=0) -fno-optimize-sibling-calls) \
+ $(call cc-param,tsan-distinguish-volatile=1)

[Marco Elver]

Clang (unlike GCC) removes reads before writes with matching addresses
in the same basic block. This is an optimization for TSAN, since writes
will always cause conflict if the preceding read would have.

However, for KCSAN we cannot rely on this option, because we apply
several special rules to writes, in particular when the
KCSAN_ASSUME_PLAIN_WRITES_ATOMIC option is selected. To avoid missing
potential data races, pass the -tsan-instrument-read-before-write option
to Clang if it is available [1].

[1] https://github.com/llvm/llvm-project/commit/151ed6aa38a3ec6c01973b35f684586b6e1c0f7e

Signed-off-by: Marco Elver <el...@google.com>
---

scripts/Makefile.kcsan | 1 +
1 file changed, 1 insertion(+)

diff --git a/scripts/Makefile.kcsan b/scripts/Makefile.kcsan
index 75d2942b9437..bd4da1af5953 100644
--- a/scripts/Makefile.kcsan
+++ b/scripts/Makefile.kcsan

@@ -13,6 +13,7 @@ endif

# of some options does not break KCSAN nor causes false positive reports.
CFLAGS_KCSAN := -fsanitize=thread \

$(call cc-option,$(call cc-param,tsan-instrument-func-entry-exit=0) -fno-optimize-sibling-calls) \
+ $(call cc-option,$(call cc-param,tsan-instrument-read-before-write=1)) \

[Marco Elver]

Some compilers incorrectly inline small __no_kcsan functions, which then
results in instrumenting the accesses. For this reason, the 'noinline'
attribute was added to __no_kcsan_or_inline. All known versions of GCC
are affected by this. Supported version of Clang are unaffected, and
never inlines a no_sanitize function.

However, the attribute 'noinline' in __no_kcsan_or_inline causes
unexpected code generation in functions that are __no_kcsan and call a
__no_kcsan_or_inline function.

In certain situations it is expected that the __no_kcsan_or_inline
function is actually inlined by the __no_kcsan function, and *no* calls
are emitted. By removing the 'noinline' attribute we give the compiler
the ability to inline and generate the expected code in __no_kcsan
functions.

Link: https://lkml.kernel.org/r/CANpmjNNOpJk0tprXKB_deiNA...@mail.gmail.com

Signed-off-by: Marco Elver <el...@google.com>
---

include/linux/compiler.h | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index e24cc3a2bc3e..17c98b215572 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h

@@ -276,11 +276,9 @@ do { \
#ifdef __SANITIZE_THREAD__
/*

* Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in

- * compilation units where instrumentation is disabled. The attribute 'noinline'
- * is required for older compilers, where implicit inlining of very small
- * functions renders __no_sanitize_thread ineffective.

+ * compilation units where instrumentation is disabled.
*/
-# define __no_kcsan_or_inline __no_kcsan noinline notrace __maybe_unused
+# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused

# define __no_sanitize_or_inline __no_kcsan_or_inline
#else

[Marco Elver]

Signed-off-by: Marco Elver <el...@google.com>
---

lib/Kconfig.kcsan | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index a7276035ca0d..3f3b5bca7a8f 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -3,6 +3,12 @@
config HAVE_ARCH_KCSAN
bool

+config HAVE_KCSAN_COMPILER
+ def_bool CC_IS_CLANG && $(cc-option,-fsanitize=thread -mllvm -tsan-distinguish-volatile=1)
+ help
+ For the list of compilers that support KCSAN, please see
+ <file:Documentation/dev-tools/kcsan.rst>.
+
config KCSAN_KCOV_BROKEN
def_bool KCOV && CC_HAS_SANCOV_TRACE_PC
depends on CC_IS_CLANG
@@ -15,7 +21,8 @@ config KCSAN_KCOV_BROKEN

menuconfig KCSAN
bool "KCSAN: dynamic data race detector"

- depends on HAVE_ARCH_KCSAN && DEBUG_KERNEL && !KASAN
+ depends on HAVE_ARCH_KCSAN && HAVE_KCSAN_COMPILER
+ depends on DEBUG_KERNEL && !KASAN

depends on !KCSAN_KCOV_BROKEN
select STACKTRACE
help

--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

Signed-off-by: Marco Elver <el...@google.com>
---

[Marco Elver]

The volatile accesses no longer need to be wrapped in data_race(),

because we require compilers that emit instrumentation distinguishing

volatile accesses. Consequently, we also no longer require the explicit
kcsan_check_atomic*(), since the compiler emits instrumentation
distinguishing the volatile accesses. Finally, simplify
__READ_ONCE_SCALAR and remove __WRITE_ONCE_SCALAR.

Signed-off-by: Marco Elver <el...@google.com>
---

v2:
* Remove unnecessary kcsan_check_atomic*() in *_ONCE.

* Simplify __READ_ONCE_SCALAR and remove __WRITE_ONCE_SCALAR. This
effectively restores Will Deacon's pre-KCSAN version:
https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/tree/include/linux/compiler.h?h=rwonce/cleanup#n202

---
include/linux/compiler.h | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 17c98b215572..7444f026eead 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -228,9 +228,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,

#define __READ_ONCE_SCALAR(x) \
({ \
- typeof(x) *__xp = &(x); \

- __unqual_scalar_typeof(x) __x = data_race(__READ_ONCE(*__xp)); \

- kcsan_check_atomic_read(__xp, sizeof(*__xp)); \
+ __unqual_scalar_typeof(x) __x = __READ_ONCE(x); \
smp_read_barrier_depends(); \
(typeof(x))__x; \
})
@@ -246,17 +244,10 @@ do { \
*(volatile typeof(x) *)&(x) = (val); \
} while (0)

-#define __WRITE_ONCE_SCALAR(x, val) \
-do { \
- typeof(x) *__xp = &(x); \
- kcsan_check_atomic_write(__xp, sizeof(*__xp)); \

- data_race(({ __WRITE_ONCE(*__xp, val); 0; })); \

-} while (0)
-
#define WRITE_ONCE(x, val) \
do { \
compiletime_assert_rwonce_type(x); \
- __WRITE_ONCE_SCALAR(x, val); \
+ __WRITE_ONCE(x, val); \
} while (0)

#ifdef CONFIG_KASAN
--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

It appears that compilers have trouble with nested statements
expressions, as such make the data_race() macro be only a single
statement expression. This will help us avoid potential problems in
future as its usage increases.

Link: https://lkml.kernel.org/r/20200520221...@zn.tnic

Signed-off-by: Marco Elver <el...@google.com>
---
v2:

* Add patch to series in response to above linked discussion.
---
include/linux/compiler.h | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 7444f026eead..1f9bd9f35368 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -211,12 +211,11 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
*/
#define data_race(expr) \
({ \
+ __unqual_scalar_typeof(({ expr; })) __v; \
__kcsan_disable_current(); \
- ({ \
- __unqual_scalar_typeof(({ expr; })) __v = ({ expr; }); \
- __kcsan_enable_current(); \
- __v; \
- }); \
+ __v = ({ expr; }); \
+ __kcsan_enable_current(); \
+ __v; \
})

/*
--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

Cleanup and move the KASAN and KCSAN related function attributes to
compiler_types.h, where the rest of the same kind live.

No functional change intended.

Signed-off-by: Marco Elver <el...@google.com>
---

include/linux/compiler.h | 29 -----------------------------
include/linux/compiler_types.h | 29 +++++++++++++++++++++++++++++

2 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 1f9bd9f35368..8d3d03f9d562 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -249,35 +249,6 @@ do { \
__WRITE_ONCE(x, val); \

} while (0)

-#ifdef CONFIG_KASAN
-/*
- * We can't declare function 'inline' because __no_sanitize_address conflicts
- * with inlining. Attempt to inline it may cause a build failure.
- * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
- * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
- */
-# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
-# define __no_sanitize_or_inline __no_kasan_or_inline
-#else
-# define __no_kasan_or_inline __always_inline
-#endif
-
-#define __no_kcsan __no_sanitize_thread
-#ifdef __SANITIZE_THREAD__
-/*

- * Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in

- * compilation units where instrumentation is disabled.

- */
-# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused
-# define __no_sanitize_or_inline __no_kcsan_or_inline
-#else
-# define __no_kcsan_or_inline __always_inline
-#endif
-
-#ifndef __no_sanitize_or_inline
-#define __no_sanitize_or_inline __always_inline
-#endif
-
static __no_sanitize_or_inline
unsigned long __read_once_word_nocheck(const void *addr)
{

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 6ed0612bc143..b190a12e7089 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -167,6 +167,35 @@ struct ftrace_likely_data {
*/
#define noinline_for_stack noinline

+#ifdef CONFIG_KASAN
+/*
+ * We can't declare function 'inline' because __no_sanitize_address conflicts
+ * with inlining. Attempt to inline it may cause a build failure.
+ * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
+ * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
+ */
+# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
+# define __no_sanitize_or_inline __no_kasan_or_inline
+#else
+# define __no_kasan_or_inline __always_inline
+#endif
+
+#define __no_kcsan __no_sanitize_thread
+#ifdef __SANITIZE_THREAD__
+/*

+ * Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in

+ * compilation units where instrumentation is disabled.

+ */

+# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused

[Marco Elver]

Like is done for KCSAN, for KASAN we should also use __always_inline in
compilation units that have instrumentation disabled
(KASAN_SANITIZE_foo.o := n). Adds common documentation for KASAN and
KCSAN explaining the attribute.

Signed-off-by: Marco Elver <el...@google.com>
---

include/linux/compiler_types.h | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index b190a12e7089..5faf68eae204 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -167,7 +167,14 @@ struct ftrace_likely_data {
*/
#define noinline_for_stack noinline

-#ifdef CONFIG_KASAN
+/*
+ * Sanitizer helper attributes: Because using __always_inline and
+ * __no_sanitize_* conflict, provide helper attributes that will either expand
+ * to __no_sanitize_* in compilation units where instrumentation is enabled
+ * (__SANITIZE_*__), or __always_inline in compilation units without
+ * instrumentation (__SANITIZE_*__ undefined).
+ */
+#ifdef __SANITIZE_ADDRESS__
/*

* We can't declare function 'inline' because __no_sanitize_address conflicts

* with inlining. Attempt to inline it may cause a build failure.

@@ -182,10 +189,6 @@ struct ftrace_likely_data {

#define __no_kcsan __no_sanitize_thread

#ifdef __SANITIZE_THREAD__
-/*
- * Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in
- * compilation units where instrumentation is disabled.
- */

# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused

# define __no_sanitize_or_inline __no_kcsan_or_inline
#else

--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

On Fri, 15 May 2020 at 17:03, Marco Elver <el...@google.com> wrote:
>
> This patch series is the conclusion to [1], where we determined that due
> to various interactions with no_sanitize attributes and the new
> {READ,WRITE}_ONCE(), KCSAN will require Clang 11 or later. Other
> sanitizers are largely untouched, and only KCSAN now has a hard
> dependency on Clang 11. To test, a recent Clang development version will
> suffice [2]. While a little inconvenient for now, it is hoped that in
> future we may be able to fix GCC and re-enable GCC support.
>
> The patch "kcsan: Restrict supported compilers" contains a detailed list
> of requirements that led to this decision.
>
> Most of the patches are related to KCSAN, however, the first patch also
> includes an UBSAN related fix and is a dependency for the remaining
> ones. The last 2 patches clean up the attributes by moving them to the
> right place, and fix KASAN's way of defining __no_kasan_or_inline,
> making it consistent with KCSAN.
>
> The series has been tested by running kcsan-test several times and
> completed successfully.
>
> [1] https://lkml.kernel.org/r/CANpmjNOGFqhtDa9wWpXs2kzt...@mail.gmail.com
> [2] https://github.com/llvm/llvm-project
>

Superseded by v2:
https://lkml.kernel.org/r/20200521110854...@google.com

[Will Deacon]

Having a 16-byte case seems a bit weird to me, but I guess clang needs this
for some reason?

Will

[Marco Elver]

Yes, the emitted fixed-size instrumentation is up to 16 bytes, so
we'll need it (for both volatile and non-volatile -- otherwise we'll
get linker errors). It doesn't mean we'll consider 16 byte volatile
accesses as atomic, because of the size check to compute is_atomic
above.

Thanks,
-- Marco

[Will Deacon]

Hopefully it doesn't matter, but this will run into issues with 'const'
non-scalar expressions.

Will

[Will Deacon]

On Thu, May 21, 2020 at 01:08:50PM +0200, Marco Elver wrote:
> Signed-off-by: Marco Elver <el...@google.com>
> ---
> Documentation/dev-tools/kcsan.rst | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)

-ENOCOMMITMSG

Will

[Marco Elver]

Oops. Ok, then there will be a v3.

> Will
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20200521133322.GC6608%40willie-the-truck.

[Will Deacon]

On Thu, May 21, 2020 at 01:08:43PM +0200, Marco Elver wrote:
> This patch series is the conclusion to [1], where we determined that due
> to various interactions with no_sanitize attributes and the new
> {READ,WRITE}_ONCE(), KCSAN will require Clang 11 or later. Other
> sanitizers are largely untouched, and only KCSAN now has a hard
> dependency on Clang 11. To test, a recent Clang development version will
> suffice [2]. While a little inconvenient for now, it is hoped that in
> future we may be able to fix GCC and re-enable GCC support.
>
> The patch "kcsan: Restrict supported compilers" contains a detailed list
> of requirements that led to this decision.
>
> Most of the patches are related to KCSAN, however, the first patch also
> includes an UBSAN related fix and is a dependency for the remaining
> ones. The last 2 patches clean up the attributes by moving them to the
> right place, and fix KASAN's way of defining __no_kasan_or_inline,
> making it consistent with KCSAN.
>
> The series has been tested by running kcsan-test several times and
> completed successfully.

I've left a few minor comments, but the only one that probably needs a bit
of thought is using data_race() with const non-scalar expressions, since I
think that's now prohibited by these changes. We don't have too many
data_race() users yet, so probably not a big deal, but worth bearing in
mind.

Other than that,

Acked-by: Will Deacon <wi...@kernel.org>

Thanks!

Will

[Marco Elver]

Good point. We could move the kcsan_disable_current() into ({
__kcsan_disable_current(); expr; }).

Will fix for v3.

Thanks,
-- Marco

> Will
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20200521133150.GB6608%40willie-the-truck.

[Marco Elver]

If you don't mind, I'll do a v3 with that fixed.

> Other than that,
>
> Acked-by: Will Deacon <wi...@kernel.org>

Thank you!

-- Marco

> Thanks!
>
> Will

[Will Deacon]

Works for me, thanks.

Will

[Marco Elver]

This patch series is the conclusion to [1], where we determined that due
to various interactions with no_sanitize attributes and the new
{READ,WRITE}_ONCE(), KCSAN will require Clang 11 or later. Other
sanitizers are largely untouched, and only KCSAN now has a hard
dependency on Clang 11. To test, a recent Clang development version will
suffice [2]. While a little inconvenient for now, it is hoped that in
future we may be able to fix GCC and re-enable GCC support.

The patch "kcsan: Restrict supported compilers" contains a detailed list
of requirements that led to this decision.

Most of the patches are related to KCSAN, however, the first patch also
includes an UBSAN related fix and is a dependency for the remaining
ones. The last 2 patches clean up the attributes by moving them to the
right place, and fix KASAN's way of defining __no_kasan_or_inline,
making it consistent with KCSAN.

The series has been tested by running kcsan-test several times and
completed successfully.

[1] https://lkml.kernel.org/r/CANpmjNOGFqhtDa9wWpXs2kzt...@mail.gmail.com
[2] https://github.com/llvm/llvm-project

v3:
* data_race() fix for 'const' non-scalar expressions.
* Add a missing commit message.
* Add Will's Acked-by.

v2: https://lkml.kernel.org/r/20200521110854...@google.com

* Remove unnecessary kcsan_check_atomic in ONCE.

* Simplify __READ_ONCE_SCALAR and remove __WRITE_ONCE_SCALAR. This
effectively restores Will Deacon's pre-KCSAN version:
https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/tree/include/linux/compiler.h?h=rwonce/cleanup#n202

* Introduce patch making data_race() a single statement expression in
response to apparent issues that compilers are having with nested
statement expressions.

Arnd Bergmann (1):
ubsan, kcsan: don't combine sanitizer with kcov on clang

Marco Elver (10):
kcsan: Avoid inserting __tsan_func_entry/exit if possible
kcsan: Support distinguishing volatile accesses
kcsan: Pass option tsan-instrument-read-before-write to Clang
kcsan: Remove 'noinline' from __no_kcsan_or_inline
kcsan: Restrict supported compilers
kcsan: Update Documentation to change supported compilers
READ_ONCE, WRITE_ONCE: Remove data_race() and unnecessary checks
data_race: Avoid nested statement expression
compiler.h: Move function attributes to compiler_types.h
compiler_types.h, kasan: Use __SANITIZE_ADDRESS__ instead of
CONFIG_KASAN to decide inlining

Documentation/dev-tools/kcsan.rst | 9 +-----

include/linux/compiler.h | 54 ++++---------------------------
include/linux/compiler_types.h | 32 ++++++++++++++++++
kernel/kcsan/core.c | 43 ++++++++++++++++++++++++

lib/Kconfig.kcsan | 20 +++++++++++-
lib/Kconfig.ubsan | 11 +++++++
scripts/Makefile.kcsan | 15 ++++++++-

7 files changed, 127 insertions(+), 57 deletions(-)

--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

From: Arnd Bergmann <ar...@arndb.de>

Clang does not allow -fsanitize-coverage=trace-{pc,cmp} together
with -fsanitize=bounds or with ubsan:

clang: error: argument unused during compilation: '-fsanitize-coverage=trace-pc' [-Werror,-Wunused-command-line-argument]
clang: error: argument unused during compilation: '-fsanitize-coverage=trace-cmp' [-Werror,-Wunused-command-line-argument]

To avoid the warning, check whether clang can handle this correctly
or disallow ubsan and kcsan when kcov is enabled.

Link: https://bugs.llvm.org/show_bug.cgi?id=45831
Link: https://lore.kernel.org/lkml/20200505142341...@arndb.de
Acked-by: Marco Elver <el...@google.com>
Signed-off-by: Arnd Bergmann <ar...@arndb.de>

Signed-off-by: Marco Elver <el...@google.com>
---

This patch is already in -rcu tree, but since since the series is based
on -tip, to avoid conflict it is required for the subsequent patches.
---
lib/Kconfig.kcsan | 11 +++++++++++
lib/Kconfig.ubsan | 11 +++++++++++
2 files changed, 22 insertions(+)

diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index ea28245c6c1d..a7276035ca0d 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -3,9 +3,20 @@
config HAVE_ARCH_KCSAN
bool

+config KCSAN_KCOV_BROKEN
+ def_bool KCOV && CC_HAS_SANCOV_TRACE_PC
+ depends on CC_IS_CLANG
+ depends on !$(cc-option,-Werror=unused-command-line-argument -fsanitize=thread -fsanitize-coverage=trace-pc)
+ help
+ Some versions of clang support either KCSAN and KCOV but not the
+ combination of the two.
+ See https://bugs.llvm.org/show_bug.cgi?id=45831 for the status
+ in newer releases.
+

menuconfig KCSAN
bool "KCSAN: dynamic data race detector"

depends on HAVE_ARCH_KCSAN && DEBUG_KERNEL && !KASAN

[Marco Elver]

To avoid inserting __tsan_func_{entry,exit}, add option if supported by
compiler. Currently only Clang can be told to not emit calls to these
functions. It is safe to not emit these, since KCSAN does not rely on
them.

Note that, if we disable __tsan_func_{entry,exit}(), we need to disable
tail-call optimization in sanitized compilation units, as otherwise we
may skip frames in the stack trace; in particular when the tail called
function is one of the KCSAN's runtime functions, and a report is
generated, might we miss the function where the actual access occurred.
Since __tsan_func_{entry,exit}() insertion effectively disabled
tail-call optimization, there should be no observable change. [This was
caught and confirmed with kcsan-test & UNWINDER_ORC.]

Acked-by: Will Deacon <wi...@kernel.org>

Signed-off-by: Marco Elver <el...@google.com>
---

scripts/Makefile.kcsan | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/scripts/Makefile.kcsan b/scripts/Makefile.kcsan
index caf1111a28ae..20337a7ecf54 100644
--- a/scripts/Makefile.kcsan
+++ b/scripts/Makefile.kcsan

@@ -1,6 +1,15 @@
# SPDX-License-Identifier: GPL-2.0
ifdef CONFIG_KCSAN

-CFLAGS_KCSAN := -fsanitize=thread
+# GCC and Clang accept backend options differently. Do not wrap in cc-option,
+# because Clang accepts "--param" even if it is unused.
+ifdef CONFIG_CC_IS_CLANG
+cc-param = -mllvm -$(1)
+else
+cc-param = --param -$(1)
+endif
+
+CFLAGS_KCSAN := -fsanitize=thread \

+ $(call cc-option,$(call cc-param,tsan-instrument-func-entry-exit=0) -fno-optimize-sibling-calls)

[Marco Elver]

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().

Acked-by: Will Deacon <wi...@kernel.org>
Signed-off-by: Marco Elver <el...@google.com>
---

+
/*
* The below are not required by KCSAN, but can still be emitted by the
* compiler.

diff --git a/scripts/Makefile.kcsan b/scripts/Makefile.kcsan
index 20337a7ecf54..75d2942b9437 100644
--- a/scripts/Makefile.kcsan
+++ b/scripts/Makefile.kcsan

@@ -9,7 +9,10 @@ else
cc-param = --param -$(1)
endif

+# Keep most options here optional, to allow enabling more compilers if absence

+# of some options does not break KCSAN nor causes false positive reports.
CFLAGS_KCSAN := -fsanitize=thread \

- $(call cc-option,$(call cc-param,tsan-instrument-func-entry-exit=0) -fno-optimize-sibling-calls)

+ $(call cc-option,$(call cc-param,tsan-instrument-func-entry-exit=0) -fno-optimize-sibling-calls) \
+ $(call cc-param,tsan-distinguish-volatile=1)

[Marco Elver]

Clang (unlike GCC) removes reads before writes with matching addresses
in the same basic block. This is an optimization for TSAN, since writes
will always cause conflict if the preceding read would have.

However, for KCSAN we cannot rely on this option, because we apply
several special rules to writes, in particular when the
KCSAN_ASSUME_PLAIN_WRITES_ATOMIC option is selected. To avoid missing
potential data races, pass the -tsan-instrument-read-before-write option
to Clang if it is available [1].

[1] https://github.com/llvm/llvm-project/commit/151ed6aa38a3ec6c01973b35f684586b6e1c0f7e

Acked-by: Will Deacon <wi...@kernel.org>
Signed-off-by: Marco Elver <el...@google.com>
---

scripts/Makefile.kcsan | 1 +
1 file changed, 1 insertion(+)

diff --git a/scripts/Makefile.kcsan b/scripts/Makefile.kcsan
index 75d2942b9437..bd4da1af5953 100644
--- a/scripts/Makefile.kcsan
+++ b/scripts/Makefile.kcsan

@@ -13,6 +13,7 @@ endif

# of some options does not break KCSAN nor causes false positive reports.
CFLAGS_KCSAN := -fsanitize=thread \

$(call cc-option,$(call cc-param,tsan-instrument-func-entry-exit=0) -fno-optimize-sibling-calls) \
+ $(call cc-option,$(call cc-param,tsan-instrument-read-before-write=1)) \

[Marco Elver]

Some compilers incorrectly inline small __no_kcsan functions, which then
results in instrumenting the accesses. For this reason, the 'noinline'
attribute was added to __no_kcsan_or_inline. All known versions of GCC
are affected by this. Supported version of Clang are unaffected, and
never inlines a no_sanitize function.

However, the attribute 'noinline' in __no_kcsan_or_inline causes
unexpected code generation in functions that are __no_kcsan and call a
__no_kcsan_or_inline function.

In certain situations it is expected that the __no_kcsan_or_inline
function is actually inlined by the __no_kcsan function, and *no* calls
are emitted. By removing the 'noinline' attribute we give the compiler
the ability to inline and generate the expected code in __no_kcsan
functions.

Link: https://lkml.kernel.org/r/CANpmjNNOpJk0tprXKB_deiNA...@mail.gmail.com

Acked-by: Will Deacon <wi...@kernel.org>
Signed-off-by: Marco Elver <el...@google.com>
---

include/linux/compiler.h | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index e24cc3a2bc3e..17c98b215572 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h

@@ -276,11 +276,9 @@ do { \
#ifdef __SANITIZE_THREAD__
/*

* Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in

- * compilation units where instrumentation is disabled. The attribute 'noinline'
- * is required for older compilers, where implicit inlining of very small
- * functions renders __no_sanitize_thread ineffective.

+ * compilation units where instrumentation is disabled.
*/
-# define __no_kcsan_or_inline __no_kcsan noinline notrace __maybe_unused
+# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused

# define __no_sanitize_or_inline __no_kcsan_or_inline
#else

[Marco Elver]

Acked-by: Will Deacon <wi...@kernel.org>
Signed-off-by: Marco Elver <el...@google.com>
---

lib/Kconfig.kcsan | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index a7276035ca0d..3f3b5bca7a8f 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -3,6 +3,12 @@
config HAVE_ARCH_KCSAN
bool

+config HAVE_KCSAN_COMPILER
+ def_bool CC_IS_CLANG && $(cc-option,-fsanitize=thread -mllvm -tsan-distinguish-volatile=1)
+ help
+ For the list of compilers that support KCSAN, please see
+ <file:Documentation/dev-tools/kcsan.rst>.
+
config KCSAN_KCOV_BROKEN
def_bool KCOV && CC_HAS_SANCOV_TRACE_PC
depends on CC_IS_CLANG
@@ -15,7 +21,8 @@ config KCSAN_KCOV_BROKEN

menuconfig KCSAN
bool "KCSAN: dynamic data race detector"

- depends on HAVE_ARCH_KCSAN && DEBUG_KERNEL && !KASAN
+ depends on HAVE_ARCH_KCSAN && HAVE_KCSAN_COMPILER
+ depends on DEBUG_KERNEL && !KASAN

depends on !KCSAN_KCOV_BROKEN
select STACKTRACE
help

--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

Document change in required compiler version for KCSAN, and remove the
now redundant note about __no_kcsan and inlining problems with older
compilers.

Acked-by: Will Deacon <wi...@kernel.org>
Signed-off-by: Marco Elver <el...@google.com>
---

v3:
* Add missing commit message.

---
Documentation/dev-tools/kcsan.rst | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)

[Marco Elver]

The volatile accesses no longer need to be wrapped in data_race(),
because we require compilers that emit instrumentation distinguishing
volatile accesses. Consequently, we also no longer require the explicit
kcsan_check_atomic*(), since the compiler emits instrumentation
distinguishing the volatile accesses. Finally, simplify
__READ_ONCE_SCALAR and remove __WRITE_ONCE_SCALAR.

Acked-by: Will Deacon <wi...@kernel.org>
Signed-off-by: Marco Elver <el...@google.com>
---

v2:
* Remove unnecessary kcsan_check_atomic*() in *_ONCE.

* Simplify __READ_ONCE_SCALAR and remove __WRITE_ONCE_SCALAR. This
effectively restores Will Deacon's pre-KCSAN version:
https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/tree/include/linux/compiler.h?h=rwonce/cleanup#n202

---
include/linux/compiler.h | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 17c98b215572..7444f026eead 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h

[Marco Elver]

It appears that compilers have trouble with nested statement

expressions. Therefore remove one level of statement expression nesting
from the data_race() macro. This will help us avoid potential problems

in future as its usage increases.

Link: https://lkml.kernel.org/r/20200520221...@zn.tnic

Acked-by: Will Deacon <wi...@kernel.org>
Signed-off-by: Marco Elver <el...@google.com>
---

v3:
* Fix for 'const' non-scalar expressions.

v2:
* Add patch to series in response to above linked discussion.
---

include/linux/compiler.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 7444f026eead..379a5077e9c6 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -211,12 +211,12 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
*/
#define data_race(expr) \
({ \
- __kcsan_disable_current(); \

- ({ \
- __unqual_scalar_typeof(({ expr; })) __v = ({ expr; }); \
- __kcsan_enable_current(); \
- __v; \

+ __unqual_scalar_typeof(({ expr; })) __v = ({ \
+ __kcsan_disable_current(); \
+ expr; \

}); \
+ __kcsan_enable_current(); \
+ __v; \

})

/*
--
2.26.2.761.g0e0b3e54be-goog

[Marco Elver]

Cleanup and move the KASAN and KCSAN related function attributes to
compiler_types.h, where the rest of the same kind live.

No functional change intended.

Acked-by: Will Deacon <wi...@kernel.org>
Signed-off-by: Marco Elver <el...@google.com>
---

include/linux/compiler.h | 29 -----------------------------
include/linux/compiler_types.h | 29 +++++++++++++++++++++++++++++

2 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 379a5077e9c6..652aee025c89 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -250,35 +250,6 @@ do { \

__WRITE_ONCE(x, val); \
} while (0)

-#ifdef CONFIG_KASAN
-/*
- * We can't declare function 'inline' because __no_sanitize_address conflicts
- * with inlining. Attempt to inline it may cause a build failure.
- * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
- * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
- */
-# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
-# define __no_sanitize_or_inline __no_kasan_or_inline
-#else
-# define __no_kasan_or_inline __always_inline
-#endif
-
-#define __no_kcsan __no_sanitize_thread
-#ifdef __SANITIZE_THREAD__
-/*

- * Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in

- * compilation units where instrumentation is disabled.

- */
-# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused
-# define __no_sanitize_or_inline __no_kcsan_or_inline
-#else
-# define __no_kcsan_or_inline __always_inline
-#endif
-
-#ifndef __no_sanitize_or_inline
-#define __no_sanitize_or_inline __always_inline
-#endif
-
static __no_sanitize_or_inline
unsigned long __read_once_word_nocheck(const void *addr)
{

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 6ed0612bc143..b190a12e7089 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -167,6 +167,35 @@ struct ftrace_likely_data {
*/
#define noinline_for_stack noinline

+#ifdef CONFIG_KASAN
+/*
+ * We can't declare function 'inline' because __no_sanitize_address conflicts
+ * with inlining. Attempt to inline it may cause a build failure.
+ * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
+ * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
+ */
+# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
+# define __no_sanitize_or_inline __no_kasan_or_inline
+#else
+# define __no_kasan_or_inline __always_inline
+#endif
+
+#define __no_kcsan __no_sanitize_thread
+#ifdef __SANITIZE_THREAD__
+/*

+ * Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in

+ * compilation units where instrumentation is disabled.

+ */

+# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused

[Marco Elver]

Like is done for KCSAN, for KASAN we should also use __always_inline in
compilation units that have instrumentation disabled
(KASAN_SANITIZE_foo.o := n). Adds common documentation for KASAN and
KCSAN explaining the attribute.

Acked-by: Will Deacon <wi...@kernel.org>
Signed-off-by: Marco Elver <el...@google.com>
---

include/linux/compiler_types.h | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index b190a12e7089..5faf68eae204 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -167,7 +167,14 @@ struct ftrace_likely_data {
*/
#define noinline_for_stack noinline

-#ifdef CONFIG_KASAN
+/*
+ * Sanitizer helper attributes: Because using __always_inline and
+ * __no_sanitize_* conflict, provide helper attributes that will either expand
+ * to __no_sanitize_* in compilation units where instrumentation is enabled
+ * (__SANITIZE_*__), or __always_inline in compilation units without
+ * instrumentation (__SANITIZE_*__ undefined).
+ */
+#ifdef __SANITIZE_ADDRESS__
/*

* We can't declare function 'inline' because __no_sanitize_address conflicts

* with inlining. Attempt to inline it may cause a build failure.

@@ -182,10 +189,6 @@ struct ftrace_likely_data {

#define __no_kcsan __no_sanitize_thread

#ifdef __SANITIZE_THREAD__
-/*
- * Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in
- * compilation units where instrumentation is disabled.
- */

# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused

# define __no_sanitize_or_inline __no_kcsan_or_inline
#else

--
2.26.2.761.g0e0b3e54be-goog

[Nick Desaulniers]

On Thu, May 21, 2020 at 7:22 AM 'Marco Elver' via Clang Built Linux
<clang-bu...@googlegroups.com> wrote:
>
> It appears that compilers have trouble with nested statement
> expressions. Therefore remove one level of statement expression nesting
> from the data_race() macro. This will help us avoid potential problems
> in future as its usage increases.
>
> Link: https://lkml.kernel.org/r/20200520221...@zn.tnic
> Acked-by: Will Deacon <wi...@kernel.org>
> Signed-off-by: Marco Elver <el...@google.com>

Thanks Marco, I can confirm this series fixes the significant build
time regressions.

Tested-by: Nick Desaulniers <ndesau...@google.com>

More measurements in: https://github.com/ClangBuiltLinux/linux/issues/1032

Might want:
Reported-by: Borislav Petkov <b...@suse.de>
Reported-by: Nathan Chancellor <natecha...@gmail.com>
too.

> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-li...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200521142047.169334-10-elver%40google.com.



--
Thanks,
~Nick Desaulniers

[Borislav Petkov]

On Thu, May 21, 2020 at 04:20:39PM +0200, Marco Elver wrote:
> diff --git a/scripts/Makefile.kcsan b/scripts/Makefile.kcsan
> index 20337a7ecf54..75d2942b9437 100644
> --- a/scripts/Makefile.kcsan
> +++ b/scripts/Makefile.kcsan
> @@ -9,7 +9,10 @@ else
> cc-param = --param -$(1)
> endif
>
> +# Keep most options here optional, to allow enabling more compilers if absence
> +# of some options does not break KCSAN nor causes false positive reports.
> CFLAGS_KCSAN := -fsanitize=thread \
> - $(call cc-option,$(call cc-param,tsan-instrument-func-entry-exit=0) -fno-optimize-sibling-calls)
> + $(call cc-option,$(call cc-param,tsan-instrument-func-entry-exit=0) -fno-optimize-sibling-calls) \
> + $(call cc-param,tsan-distinguish-volatile=1)

gcc 9 doesn't like this:

cc1: error: invalid --param name ‘-tsan-distinguish-volatile’
make[1]: *** [scripts/Makefile.build:100: scripts/mod/devicetable-offsets.s] Error 1
make[1]: *** Waiting for unfinished jobs....
cc1: error: invalid --param name ‘-tsan-distinguish-volatile’
make[1]: *** [scripts/Makefile.build:267: scripts/mod/empty.o] Error 1
make: *** [Makefile:1141: prepare0] Error 2
make: *** Waiting for unfinished jobs....

git grep "tsan-distinguish-volatile" in gcc's git doesn't give anything.

Hmm.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[Marco Elver]

Yeah, my patch for GCC is still pending. But we probably need more
fixes for GCC, before we can re-enable it.

We restrict supported compilers later in the series:
https://lore.kernel.org/lkml/20200521142047...@google.com/

More background is also in the cover letter:
https://lore.kernel.org/lkml/20200521142047...@google.com/

Thanks,
-- Marco

[Borislav Petkov]

On Fri, May 22, 2020 at 12:34:00PM +0200, Marco Elver wrote:
> Yeah, my patch for GCC is still pending. But we probably need more
> fixes for GCC, before we can re-enable it.
>
> We restrict supported compilers later in the series:
> https://lore.kernel.org/lkml/20200521142047...@google.com/

Yah, tglx just pointed me to it. I'll move 6/11 up in the series.

Just a tip for the future: the idea is to have the kernel build
successfully at each patch so that bisection doesn't break.

Thx.

[Peter Zijlstra]

On Thu, May 21, 2020 at 04:20:36PM +0200, Marco Elver wrote:
> Arnd Bergmann (1):
> ubsan, kcsan: don't combine sanitizer with kcov on clang
>
> Marco Elver (10):
> kcsan: Avoid inserting __tsan_func_entry/exit if possible
> kcsan: Support distinguishing volatile accesses
> kcsan: Pass option tsan-instrument-read-before-write to Clang
> kcsan: Remove 'noinline' from __no_kcsan_or_inline
> kcsan: Restrict supported compilers
> kcsan: Update Documentation to change supported compilers
> READ_ONCE, WRITE_ONCE: Remove data_race() and unnecessary checks
> data_race: Avoid nested statement expression
> compiler.h: Move function attributes to compiler_types.h
> compiler_types.h, kasan: Use __SANITIZE_ADDRESS__ instead of
> CONFIG_KASAN to decide inlining
>
> Documentation/dev-tools/kcsan.rst | 9 +-----
> include/linux/compiler.h | 54 ++++---------------------------
> include/linux/compiler_types.h | 32 ++++++++++++++++++
> kernel/kcsan/core.c | 43 ++++++++++++++++++++++++
> lib/Kconfig.kcsan | 20 +++++++++++-
> lib/Kconfig.ubsan | 11 +++++++
> scripts/Makefile.kcsan | 15 ++++++++-
> 7 files changed, 127 insertions(+), 57 deletions(-)

LTGM

Acked-by: Peter Zijlstra (Intel) <pet...@infradead.org>

[Arnd Bergmann]

On Thu, May 21, 2020 at 10:21 PM 'Nick Desaulniers' via Clang Built

Linux <clang-bu...@googlegroups.com> wrote:
>
> On Thu, May 21, 2020 at 7:22 AM 'Marco Elver' via Clang Built Linux
> <clang-bu...@googlegroups.com> wrote:
> >
> > It appears that compilers have trouble with nested statement
> > expressions. Therefore remove one level of statement expression nesting
> > from the data_race() macro. This will help us avoid potential problems
> > in future as its usage increases.
> >
> > Link: https://lkml.kernel.org/r/20200520221...@zn.tnic
> > Acked-by: Will Deacon <wi...@kernel.org>
> > Signed-off-by: Marco Elver <el...@google.com>
>
> Thanks Marco, I can confirm this series fixes the significant build
> time regressions.
>
> Tested-by: Nick Desaulniers <ndesau...@google.com>
>
> More measurements in: https://github.com/ClangBuiltLinux/linux/issues/1032
>
> Might want:
> Reported-by: Borislav Petkov <b...@suse.de>
> Reported-by: Nathan Chancellor <natecha...@gmail.com>
> too.

I find this patch only solves half the problem: it's much faster than
without the
patch, but still much slower than the current mainline version. As far as I'm
concerned, I think the build speed regression compared to mainline is not yet
acceptable, and we should try harder.

I have not looked too deeply at it yet, but this is what I found from looking
at a file in a randconfig build:

Configuration: see https://pastebin.com/raw/R9erCwNj

== Current linux-next ==
with "data_race: Avoid nested statement expression"
and "compiler.h: Remove data_race() and unnecessary checks from
{READ,WRITE}_ONCE()"

$ touch fs/ocfs2/journal.c ; cp
../arch/x86/configs/0xFFA843AA_defconfig obj-x86/.config ; perf stat
make olddefconfig O=obj-x86/ CC=clang-11 fs/ocfs2/journal.i ARCH=x86
-skj30 ; wc obj-x86/fs/ocfs2/journal.i
48741 552950 9010050 obj-x86/fs/ocfs2/journal.i
real 0m12.514s
user 0m10.270s
sys 0m2.668s

== Same tree, without those two ==

$ touch fs/ocfs2/journal.c cp ../arch/x86/configs/0xFFA843AA_defconfig
obj-x86/.config ; time make olddefconfig O=obj-x86/ CC=clang-11
fs/ocfs2/journal.i ARCH=x86 -skj30 ; wc obj-x86/fs/ocfs2/journal.i
real 1m35.968s
user 1m33.579s
sys 0m3.523s
48741 1926607 36542560 obj-x86/fs/ocfs2/journal.i

== Mainline Linux ==

$ touch fs/ocfs2/journal.c ; cp
../arch/x86/configs/0xFFA843AA_defconfig obj-x86/.config ; time make
olddefconfig O=obj-x86/ CC=clang-11 fs/ocfs2/journal.i ARCH=x86 -skj30
; wc obj-x86/fs/ocfs2/journal.i

real 0m6.529s
user 0m4.389s
sys 0m2.561s
47377 377887 4178633 obj-x86/fs/ocfs2/journal.i

So both the size of the preprocessed file and the time to preprocess it are
still twice as bad for linux-next compared to mainline. Actually compiling the
preprocessed filed is very quick, as I guess only the preprocessing seems to
use all the time.

Arnd

[Will Deacon]

So this .config actually has KCSAN enabled. Do you still see the slowdown
with that disabled? Although not ideal, having a longer compiler time when
the compiler is being asked to perform instrumentation doesn't seem like a
show-stopper to me.

Will

[Arnd Bergmann]

Yes, enabling or disabling KCSAN seems to make no difference to
compile speed in this config and source file, I still get the 12 seconds
preprocessing time and 9MB file size with KCSAN disabled, possibly
a few percent smaller/faster. I actually thought that CONFIG_FTRACE
had a bigger impact, but disabling that also just reduces the time
by a few percent rather than getting it down to the expected milliseconds.

> Although not ideal, having a longer compiler time when
> the compiler is being asked to perform instrumentation doesn't seem like a
> show-stopper to me.

I agree in general, but building an allyesconfig kernel is still an important
use case that should not take twice as long after a small kernel change
regardless of whether a new feature is used or not. (I have not actually
compared the overall build speed for allmodconfig, as this takes a really
long time at the moment)

Arnd

[Marco Elver]

Note that an 'allyesconfig' selects KASAN and not KCSAN by default.
But I think that's not relevant, since KCSAN-specific code was removed
from ONCEs. In general though, it is entirely expected that we have a
bit longer compile times when we have the instrumentation passes
enabled.

But as you pointed out, that's irrelevant, and the significant
overhead is from parsing and pre-processing. FWIW, we can probably
optimize Clang itself a bit:
https://github.com/ClangBuiltLinux/linux/issues/1032#issuecomment-633712667

Thanks,
-- Marco

[Marco Elver]

Found that optimizing __unqual_scalar_typeof makes a noticeable
difference. We could use C11's _Generic if the compiler supports it (and
all supported versions of Clang certainly do).

Could you verify if the below patch improves compile-times for you? E.g.
on fs/ocfs2/journal.c I was able to get ~40% compile-time speedup.

Thanks,
-- Marco

------ >8 ------

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 5faf68eae204..a529fa263906 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -245,7 +245,9 @@ struct ftrace_likely_data {
/*
* __unqual_scalar_typeof(x) - Declare an unqualified scalar type, leaving
* non-scalar types unchanged.
- *
+ */
+#if defined(CONFIG_CC_IS_GCC) && CONFIG_GCC_VERSION < 40900
+/*
* We build this out of a couple of helper macros in a vain attempt to
* help you keep your lunch down while reading it.
*/
@@ -267,6 +269,24 @@ struct ftrace_likely_data {
__pick_integer_type(x, int, \
__pick_integer_type(x, long, \
__pick_integer_type(x, long long, x))))))
+#else
+/*
+ * If supported, prefer C11 _Generic for better compile-times. As above, 'char'
+ * is not type-compatible with 'signed char', and we define a separate case.
+ */
+#define __scalar_type_to_expr_cases(type) \
+ type: (type)0, unsigned type: (unsigned type)0
+
+#define __unqual_scalar_typeof(x) typeof( \
+ _Generic((x), \
+ __scalar_type_to_expr_cases(char), \
+ signed char: (signed char)0, \
+ __scalar_type_to_expr_cases(short), \
+ __scalar_type_to_expr_cases(int), \
+ __scalar_type_to_expr_cases(long), \
+ __scalar_type_to_expr_cases(long long), \
+ default: (x)))
+#endif

/* Is this type a native word size -- useful for atomic operations */
#define __native_word(t) \

[Arnd Bergmann]

On Tue, May 26, 2020 at 7:33 PM 'Marco Elver' via Clang Built Linux
<clang-bu...@googlegroups.com> wrote:
> On Tue, 26 May 2020, Marco Elver wrote:
> > On Tue, 26 May 2020 at 14:19, Arnd Bergmann <ar...@arndb.de> wrote:
> > Note that an 'allyesconfig' selects KASAN and not KCSAN by default.
> > But I think that's not relevant, since KCSAN-specific code was removed
> > from ONCEs. In general though, it is entirely expected that we have a
> > bit longer compile times when we have the instrumentation passes
> > enabled.
> >
> > But as you pointed out, that's irrelevant, and the significant
> > overhead is from parsing and pre-processing. FWIW, we can probably
> > optimize Clang itself a bit:
> > https://github.com/ClangBuiltLinux/linux/issues/1032#issuecomment-633712667
>
> Found that optimizing __unqual_scalar_typeof makes a noticeable
> difference. We could use C11's _Generic if the compiler supports it (and
> all supported versions of Clang certainly do).
>
> Could you verify if the below patch improves compile-times for you? E.g.
> on fs/ocfs2/journal.c I was able to get ~40% compile-time speedup.

Yes, that brings both the preprocessed size and the time to preprocess it
with clang-11 back to where it is in mainline, and close to the speed with
gcc-10 for this particular file.

I also cross-checked with gcc-4.9 and gcc-10 and found that they do see
the same increase in the preprocessor output, but it makes little difference
for preprocessing performance on gcc.

Arnd

[Peter Zijlstra]

Yeah, this shaves around 5% off of my kernel builds. The _Atomic hack is
every so slightly faster on GCC but apparently doesn't work on clang --
also, it's disguisting :-)

Ack!

[Arnd Bergmann]

Just for reference, I've tested this against a patch I made that completely
shortcuts READ_ONCE() on anything but alpha (which needs the
read_barrier_depends()):

--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -224,18 +224,21 @@ void ftrace_likely_update(struct
ftrace_likely_data *f, int val,
* atomicity or dependency ordering guarantees. Note that this may result
* in tears!
*/
-#define __READ_ONCE(x) (*(const volatile __unqual_scalar_typeof(x) *)&(x))
+#define __READ_ONCE(x) (*(const volatile typeof(x) *)&(x))

+#ifdef CONFIG_ALPHA /* smp_read_barrier_depends is a NOP otherwise */
#define __READ_ONCE_SCALAR(x) \
({ \

__unqual_scalar_typeof(x) __x = __READ_ONCE(x); \
smp_read_barrier_depends(); \

- (typeof(x))__x; \
+ __x; \
})
+#else
+#define __READ_ONCE_SCALAR(x) __READ_ONCE(x)
+#endif

#define READ_ONCE(x) \
({ \

In the configuration I posted earlier, this produces noticeably faster
build times
patch, but yours gets most of the way: https://pastebin.com/pCwALmUD

Looking just at the "task-clock" output from 'perf stat make vmlinux -skj30'

clang-11 gcc-9
linux-next 6939594.65 msec 4191482.92 msec
Marco's patch 5399261.82 msec 3800409.58 msec
Arnd's patch 5273888.54 msec 3584550.23 msec

Arnd

[Will Deacon]

Nice! FWIW, I'm planning to have Alpha override __READ_ONCE_SCALAR()
eventually, so that smp_read_barrier_depends() can disappear forever. I
just bit off more than I can chew for 5.8 :(

However, '__unqual_scalar_typeof()' is still useful for
load-acquire/store-release on arm64, so we still need a better solution to
the build-time regression imo. I'm not fond of picking random C11 features
to accomplish that, but I also don't have any better ideas...

Is there any mileage in the clever trick from Rasmus?

https://lore.kernel.org/r/6cbc8ae1-8eb1-a5a0...@rasmusvillemoes.dk

Will

[Marco Elver]

We already use _Static_assert in the kernel, so it's not the first use
of a C11 feature.

> Is there any mileage in the clever trick from Rasmus?
>
> https://lore.kernel.org/r/6cbc8ae1-8eb1-a5a0...@rasmusvillemoes.dk

Apparently that one only works with GCC 7 or newer, and is only
properly defined behaviour since C11. It also relies on multiple
_Pragma. I'd probably take the arguably much cleaner _Generic solution
over that. ;-)

I think given that Peter and Arnd already did some testing, and it
works as intended, if you don't mind, I'll send a patch for the
_Generic version. At least that'll give us a more optimized
__unqual_scalar_typeof(). Any further optimizations to READ_ONCE()
like you mentioned then become a little less urgent.

Thanks,
-- Marco

[Arnd Bergmann]

I'd have to try, but I suspect we could force gcc-4.9 or higher to
accept it by always passing --std=gnu11 instead of --std=gnu89,
but that still wouldn't help us with gcc-4.8, and it's definitely not
something we could consider changing for v5.8.

However, if we find a solution that is nicer and faster but does
requires C11 or some other features from a newer compiler,
I think making it version dependent is a good idea and lets us
drop the worse code eventually.

> I think given that Peter and Arnd already did some testing, and it
> works as intended, if you don't mind, I'll send a patch for the
> _Generic version. At least that'll give us a more optimized
> __unqual_scalar_typeof(). Any further optimizations to READ_ONCE()
> like you mentioned then become a little less urgent.

Right. I think there is still room for optimization around here, but
for v5.8 I'm happy enough with Marco's__unqual_scalar_typeof()
change. Stephen Rothwell is probably the one who's most affected
by compile speed, so it would be good to get an Ack/Nak from him
on whether this brings speed and memory usage back to normal
for him as well.

Arnd

[Stephen Rothwell]

Hi Arnd,

On Wed, 27 May 2020 11:26:51 +0200 Arnd Bergmann <ar...@arndb.de> wrote:
>
> Right. I think there is still room for optimization around here, but
> for v5.8 I'm happy enough with Marco's__unqual_scalar_typeof()
> change. Stephen Rothwell is probably the one who's most affected
> by compile speed, so it would be good to get an Ack/Nak from him
> on whether this brings speed and memory usage back to normal
> for him as well.

Assuming you meant "[PATCH -tip] compiler_types.h: Optimize
__unqual_scalar_typeof compilation time"
https://lore.kernel.org/lkml/20200527103236...@google.com/

I did some x86_64 allmodconfig builds (as I do all day):

Linus' tree:

36884.15user 1439.31system 9:05.46elapsed 7025%CPU (0avgtext+0avgdata 500416maxresident)k
0inputs+128outputs (0major+64821256minor)pagefaults 0swaps
36878.19user 1436.60system 9:05.37elapsed 7025%CPU (0avgtext+0avgdata 494656maxresident)k
0inputs+128outputs (0major+64771097minor)pagefaults 0swaps

linux-next:

42378.58user 1513.34system 9:59.33elapsed 7323%CPU (0avgtext+0avgdata 537920maxresident)k
0inputs+384outputs (0major+65102976minor)pagefaults 0swaps
42378.38user 1509.52system 9:59.12elapsed 7325%CPU (0avgtext+0avgdata 535360maxresident)k
0inputs+384outputs (0major+65102513minor)pagefaults 0swaps

linux-next+patch:

39090.54user 1464.71system 9:17.36elapsed 7276%CPU (0avgtext+0avgdata 520576maxresident)k
0inputs+384outputs (0major+62226026minor)pagefaults 0swaps
39101.66user 1471.55system 9:18.13elapsed 7269%CPU (0avgtext+0avgdata 513856maxresident)k
0inputs+384outputs (0major+62243972minor)pagefaults 0swaps

So, it is a bit better than current linux-next, but not quita back to
Linus' tree (but that is not unexpected as there are over 12000 new
commits in -next).

$ x86_64-linux-gnu-gcc --version
x86_64-linux-gnu-gcc (Debian 9.3.0-8) 9.3.0

80 thread Power8 using -j100

--
Cheers,
Stephen Rothwell

[Peter Zijlstra]

On Thu, May 21, 2020 at 04:20:41PM +0200, Marco Elver wrote:
> Some compilers incorrectly inline small __no_kcsan functions, which then
> results in instrumenting the accesses. For this reason, the 'noinline'
> attribute was added to __no_kcsan_or_inline. All known versions of GCC
> are affected by this. Supported version of Clang are unaffected, and
> never inlines a no_sanitize function.
>
> However, the attribute 'noinline' in __no_kcsan_or_inline causes
> unexpected code generation in functions that are __no_kcsan and call a
> __no_kcsan_or_inline function.
>
> In certain situations it is expected that the __no_kcsan_or_inline
> function is actually inlined by the __no_kcsan function, and *no* calls
> are emitted. By removing the 'noinline' attribute we give the compiler
> the ability to inline and generate the expected code in __no_kcsan
> functions.

Doesn't this mean we can do the 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``::

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.
- */
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
-#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

+
#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)) {

[Marco Elver]

On Fri, 29 May 2020 at 19:08, Peter Zijlstra <pet...@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

[Peter Zijlstra]

On Fri, May 29, 2020 at 08:36:56PM +0200, Marco Elver wrote:

> > +/* 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?

Yes, and __no_sanitize_undefined and whatever else there is.

https://lkml.kernel.org/r/20200529171...@hirez.programming.kicks-ass.net

> 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.

Ok, I'll try not forget over the weekend ;-)