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

[PATCH -tip v3 00/11] 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/CANpmjNOGFqhtDa9wWpXs2kztQsSozbwsuMO5BqqW0c0g0zGfSA@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.114437-1-elver@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

[PATCH -tip v3 01/11] ubsan, kcsan: don't combine sanitizer with kcov on clang

[Marco Elver]

From: Arnd Bergmann <arnd@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.1096942-1-arnd@arndb.de
Acked-by: Marco Elver <elver@google.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Marco Elver <elver@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

[PATCH -tip v3 02/11] kcsan: Avoid inserting __tsan_func_entry/exit if possible

[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 <will@kernel.org>
Signed-off-by: Marco Elver <elver@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

[PATCH -tip v3 03/11] kcsan: Support distinguishing volatile accesses

[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 <will@kernel.org>
Signed-off-by: Marco Elver <elver@google.com>
---
v2:
* Reword Makefile comment.
---
 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..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)
 
 endif # CONFIG_KCSAN
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH -tip v3 04/11] kcsan: Pass option tsan-instrument-read-before-write to Clang

[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 <will@kernel.org>
Signed-off-by: Marco Elver <elver@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)) \
 	$(call cc-param,tsan-distinguish-volatile=1)
 
 endif # CONFIG_KCSAN
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH -tip v3 05/11] kcsan: Remove 'noinline' from __no_kcsan_or_inline

[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_deiNAv_UmmORf1-2uajLhnLWQQ1hvoA@mail.gmail.com
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Marco Elver <elver@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

[PATCH -tip v3 06/11] kcsan: Restrict supported compilers

[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_uM4aDUWE_kh3Rvbw@mail.gmail.com
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Marco Elver <elver@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

[PATCH -tip v3 07/11] kcsan: Update Documentation to change supported compilers

[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 <will@kernel.org>
Signed-off-by: Marco Elver <elver@google.com>
---
v3:
* Add missing commit message.
---
 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

[PATCH -tip v3 08/11] READ_ONCE, WRITE_ONCE: Remove data_race() and unnecessary checks

[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 <will@kernel.org>
Signed-off-by: Marco Elver <elver@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

[PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[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/20200520221712.GA21166@zn.tnic
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Marco Elver <elver@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

[PATCH -tip v3 10/11] compiler.h: Move function attributes to compiler_types.h

[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 <will@kernel.org>
Signed-off-by: Marco Elver <elver@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
+# 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

[PATCH -tip v3 11/11] compiler_types.h, kasan: Use __SANITIZE_ADDRESS__ instead of CONFIG_KASAN to decide inlining

[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 <will@kernel.org>
Signed-off-by: Marco Elver <elver@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

Re: [PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[Nick Desaulniers]

On Thu, May 21, 2020 at 7:22 AM 'Marco Elver' via Clang Built Linux
<clang-built-linux@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/20200520221712.GA21166@zn.tnic
> Acked-by: Will Deacon <will@kernel.org>
> Signed-off-by: Marco Elver <elver@google.com>

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

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

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

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

> ---
> 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
>
> --
> 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-linux+unsubscribe@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

Re: [PATCH -tip v3 03/11] kcsan: Support distinguishing volatile accesses

[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

Re: [PATCH -tip v3 03/11] kcsan: Support distinguishing volatile accesses

[Marco Elver]

On Fri, 22 May 2020 at 12:26, Borislav Petkov <bp@alien8.de> wrote:
>
> 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.

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.169334-7-elver@google.com/

More background is also in the cover letter:
https://lore.kernel.org/lkml/20200521142047.169334-1-elver@google.com/

Thanks,
-- Marco

Re: [PATCH -tip v3 03/11] kcsan: Support distinguishing volatile accesses

[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.169334-7-elver@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.

-- 
Regards/Gruss,
    Boris.

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

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

[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) <peterz@infradead.org>

[tip: locking/kcsan] compiler_types.h, kasan: Use __SANITIZE_ADDRESS__ instead of CONFIG_KASAN to decide inlining

[tip-bot2 for Marco Elver]

The following commit has been merged into the locking/kcsan branch of tip:

Commit-ID:     b91caf58f6fb88738f444cf40d247475c367de47
Gitweb:        https://git.kernel.org/tip/b91caf58f6fb88738f444cf40d247475c367de47
Author:        Marco Elver <elver@google.com>
AuthorDate:    Thu, 21 May 2020 16:20:47 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Fri, 22 May 2020 15:31:04 +02:00

compiler_types.h, kasan: Use __SANITIZE_ADDRESS__ instead of CONFIG_KASAN to decide inlining

Use __always_inline in compilation units that have instrumentation
disabled (KASAN_SANITIZE_foo.o := n) for KASAN, like it is done for
KCSAN.

Also, add common documentation for KASAN and KCSAN explaining the
attribute.

 [ bp: Massage commit message. ]

Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Will Deacon <will@kernel.org>
Link: https://lkml.kernel.org/r/20200521142047.169334-12-elver@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 b190a12..5faf68e 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

[tip: locking/kcsan] compiler.h: Move function attributes to compiler_types.h

[tip-bot2 for Marco Elver]

The following commit has been merged into the locking/kcsan branch of tip:

Commit-ID:     9a4e6db6161cc3b31c6202f8d7a9495e0c2ecda7
Gitweb:        https://git.kernel.org/tip/9a4e6db6161cc3b31c6202f8d7a9495e0c2ecda7
Author:        Marco Elver <elver@google.com>
AuthorDate:    Thu, 21 May 2020 16:20:46 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Fri, 22 May 2020 15:27:32 +02:00

compiler.h: Move function attributes to compiler_types.h

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 <elver@google.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Will Deacon <will@kernel.org>
Link: https://lkml.kernel.org/r/20200521142047.169334-11-elver@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 379a507..652aee0 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 6ed0612..b190a12 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__ */

[tip: locking/kcsan] compiler.h: Remove data_race() and unnecessary checks from {READ,WRITE}_ONCE()

[tip-bot2 for Marco Elver]

The following commit has been merged into the locking/kcsan branch of tip:

Commit-ID:     777f73c4e79106d45b304f6af0d31917864dbdf1
Gitweb:        https://git.kernel.org/tip/777f73c4e79106d45b304f6af0d31917864dbdf1
Author:        Marco Elver <elver@google.com>
AuthorDate:    Thu, 21 May 2020 16:20:44 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Fri, 22 May 2020 15:19:53 +02:00

compiler.h: Remove data_race() and unnecessary checks from {READ,WRITE}_ONCE()

The volatile accesses no longer need to be wrapped in data_race()
because compilers that emit instrumentation distinguishing volatile
accesses are required for KCSAN.

Consequently, the explicit kcsan_check_atomic*() are no longer required
either since the compiler emits instrumentation distinguishing the
volatile accesses.

Finally, simplify __READ_ONCE_SCALAR() and remove __WRITE_ONCE_SCALAR().

 [ bp: Convert commit message to passive voice. ]

Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Will Deacon <will@kernel.org>
Link: https://lkml.kernel.org/r/20200521142047.169334-9-elver@google.com
---
 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 17c98b2..7444f02 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

[tip: locking/kcsan] kcsan: Pass option tsan-instrument-read-before-write to Clang

[tip-bot2 for Marco Elver]

The following commit has been merged into the locking/kcsan branch of tip:

Commit-ID:     52dfbb97a90fbf6a9826f15a71fca37861330a13
Gitweb:        https://git.kernel.org/tip/52dfbb97a90fbf6a9826f15a71fca37861330a13
Author:        Marco Elver <elver@google.com>
AuthorDate:    Thu, 21 May 2020 16:20:40 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Fri, 22 May 2020 14:57:39 +02:00

kcsan: Pass option tsan-instrument-read-before-write to Clang

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 <elver@google.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Will Deacon <will@kernel.org>
Link: https://lkml.kernel.org/r/20200521142047.169334-5-elver@google.com
---
 scripts/Makefile.kcsan | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/Makefile.kcsan b/scripts/Makefile.kcsan
index 75d2942..bd4da1a 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)) \
 	$(call cc-param,tsan-distinguish-volatile=1)
 
 endif # CONFIG_KCSAN

[tip: locking/kcsan] kcsan: Update Documentation to change supported compilers

[tip-bot2 for Marco Elver]

The following commit has been merged into the locking/kcsan branch of tip:

Commit-ID:     345043266de282a4059bc8336e2bcdd3680cc8f0
Gitweb:        https://git.kernel.org/tip/345043266de282a4059bc8336e2bcdd3680cc8f0
Author:        Marco Elver <elver@google.com>
AuthorDate:    Thu, 21 May 2020 16:20:43 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Fri, 22 May 2020 15:13:45 +02:00

kcsan: Update Documentation to change supported compilers

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

Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Will Deacon <will@kernel.org>
Link: https://lkml.kernel.org/r/20200521142047.169334-8-elver@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 f4b5766..ce4bbd9 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``::
 

[tip: locking/kcsan] kcsan: Support distinguishing volatile accesses

[tip-bot2 for Marco Elver]

The following commit has been merged into the locking/kcsan branch of tip:

Commit-ID:     4e23395b9e97562d12b87a330a2fca3bf10c8663
Gitweb:        https://git.kernel.org/tip/4e23395b9e97562d12b87a330a2fca3bf10c8663
Author:        Marco Elver <elver@google.com>
AuthorDate:    Thu, 21 May 2020 16:20:39 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Fri, 22 May 2020 14:46:02 +02:00

kcsan: Support distinguishing volatile accesses

In the kernel, the "volatile" keyword is used in various concurrent
contexts, whether in low-level synchronization primitives or for
legacy reasons. If supported by the compiler, it will be assumed
that aligned volatile accesses up to sizeof(long long) (matching
compiletime_assert_rwonce_type()) are atomic.

Recent versions of 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 change allows removing of any explicit checks in primitives such as
READ_ONCE() and WRITE_ONCE().

 [ bp: Massage commit message a bit. ]

Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Will Deacon <will@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200521142047.169334-4-elver@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 a73a66c..15f6794 100644
--- a/kernel/kcsan/core.c
+++ b/kernel/kcsan/core.c
@@ -790,6 +790,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 20337a7..75d2942 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)
 
 endif # CONFIG_KCSAN

[tip: locking/kcsan] kcsan: Avoid inserting __tsan_func_entry/exit if possible

[tip-bot2 for Marco Elver]

The following commit has been merged into the locking/kcsan branch of tip:

Commit-ID:     3bc9e5b0725b353b921feaf2c10bb4a9f932646f
Gitweb:        https://git.kernel.org/tip/3bc9e5b0725b353b921feaf2c10bb4a9f932646f
Author:        Marco Elver <elver@google.com>
AuthorDate:    Thu, 21 May 2020 16:20:38 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Fri, 22 May 2020 14:36:19 +02:00

kcsan: Avoid inserting __tsan_func_entry/exit if possible

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, we might 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 <elver@google.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Will Deacon <will@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200521142047.169334-3-elver@google.com
---
 scripts/Makefile.kcsan | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/scripts/Makefile.kcsan b/scripts/Makefile.kcsan
index caf1111..20337a7 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

Re: [PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[Arnd Bergmann]

On Thu, May 21, 2020 at 10:21 PM 'Nick Desaulniers' via Clang Built
Linux <clang-built-linux@googlegroups.com> wrote:
>
> On Thu, May 21, 2020 at 7:22 AM 'Marco Elver' via Clang Built Linux
> <clang-built-linux@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/20200520221712.GA21166@zn.tnic
> > Acked-by: Will Deacon <will@kernel.org>
> > Signed-off-by: Marco Elver <elver@google.com>
>
> Thanks Marco, I can confirm this series fixes the significant build
> time regressions.
>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>
>
> More measurements in: https://github.com/ClangBuiltLinux/linux/issues/1032
>
> Might want:
> Reported-by: Borislav Petkov <bp@suse.de>
> Reported-by: Nathan Chancellor <natechancellor@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

Re: [PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[Will Deacon]

On Tue, May 26, 2020 at 12:42:16PM +0200, Arnd Bergmann wrote:
> On Thu, May 21, 2020 at 10:21 PM 'Nick Desaulniers' via Clang Built
> Linux <clang-built-linux@googlegroups.com> wrote:
> >
> > On Thu, May 21, 2020 at 7:22 AM 'Marco Elver' via Clang Built Linux
> > <clang-built-linux@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/20200520221712.GA21166@zn.tnic
> > > Acked-by: Will Deacon <will@kernel.org>
> > > Signed-off-by: Marco Elver <elver@google.com>
> >
> > Thanks Marco, I can confirm this series fixes the significant build
> > time regressions.
> >
> > Tested-by: Nick Desaulniers <ndesaulniers@google.com>
> >
> > More measurements in: https://github.com/ClangBuiltLinux/linux/issues/1032
> >
> > Might want:
> > Reported-by: Borislav Petkov <bp@suse.de>
> > Reported-by: Nathan Chancellor <natechancellor@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

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

Re: [PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[Arnd Bergmann]

On Tue, May 26, 2020 at 2:02 PM Will Deacon <will@kernel.org> wrote:
> On Tue, May 26, 2020 at 12:42:16PM +0200, Arnd Bergmann wrote:
> >
> > 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
>
> So this .config actually has KCSAN enabled. Do you still see the slowdown
> with that disabled?

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

Re: [PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[Marco Elver]

On Tue, 26 May 2020 at 14:19, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, May 26, 2020 at 2:02 PM Will Deacon <will@kernel.org> wrote:
> > On Tue, May 26, 2020 at 12:42:16PM +0200, Arnd Bergmann wrote:
> > >
> > > 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
> >
> > So this .config actually has KCSAN enabled. Do you still see the slowdown
> > with that disabled?
>
> 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)

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

Re: [PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[Marco Elver]

On Tue, 26 May 2020, Marco Elver wrote:

> On Tue, 26 May 2020 at 14:19, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tue, May 26, 2020 at 2:02 PM Will Deacon <will@kernel.org> wrote:
> > > On Tue, May 26, 2020 at 12:42:16PM +0200, Arnd Bergmann wrote:
> > > >
> > > > 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
> > >
> > > So this .config actually has KCSAN enabled. Do you still see the slowdown
> > > with that disabled?
> >
> > 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)
> 
> 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.

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) \

Re: [PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[Arnd Bergmann]

On Tue, May 26, 2020 at 7:33 PM 'Marco Elver' via Clang Built Linux
<clang-built-linux@googlegroups.com> wrote:
> On Tue, 26 May 2020, Marco Elver wrote:
> > On Tue, 26 May 2020 at 14:19, Arnd Bergmann <arnd@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

Re: [PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[Peter Zijlstra]

On Tue, May 26, 2020 at 07:33:12PM +0200, Marco Elver wrote:
> 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) \
> 

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!

Re: [PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[Arnd Bergmann]

On Tue, May 26, 2020 at 9:00 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, May 26, 2020 at 7:33 PM 'Marco Elver' via Clang Built Linux
> <clang-built-linux@googlegroups.com> wrote:
> > On Tue, 26 May 2020, Marco Elver wrote:
> > > On Tue, 26 May 2020 at 14:19, Arnd Bergmann <arnd@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.

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

Re: [PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[Will Deacon]

On Wed, May 27, 2020 at 01:10:00AM +0200, Arnd Bergmann wrote:
> On Tue, May 26, 2020 at 9:00 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tue, May 26, 2020 at 7:33 PM 'Marco Elver' via Clang Built Linux
> > <clang-built-linux@googlegroups.com> wrote:
> > > On Tue, 26 May 2020, Marco Elver wrote:
> > > > On Tue, 26 May 2020 at 14:19, Arnd Bergmann <arnd@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.
> 
> 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

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-a584-2081fca1c4aa@rasmusvillemoes.dk

Will

Re: [PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[Marco Elver]

On Wed, 27 May 2020 at 09:22, Will Deacon <will@kernel.org> wrote:
>
> On Wed, May 27, 2020 at 01:10:00AM +0200, Arnd Bergmann wrote:
> > On Tue, May 26, 2020 at 9:00 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > On Tue, May 26, 2020 at 7:33 PM 'Marco Elver' via Clang Built Linux
> > > <clang-built-linux@googlegroups.com> wrote:
> > > > On Tue, 26 May 2020, Marco Elver wrote:
> > > > > On Tue, 26 May 2020 at 14:19, Arnd Bergmann <arnd@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.
> >
> > 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
>
> 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...

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-a584-2081fca1c4aa@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

Re: [PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[Arnd Bergmann]

On Wed, May 27, 2020 at 9:44 AM 'Marco Elver' via Clang Built Linux
<clang-built-linux@googlegroups.com> wrote:
> On Wed, 27 May 2020 at 09:22, Will Deacon <will@kernel.org> wrote:
> >
> > 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...
>
> 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-a584-2081fca1c4aa@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'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

Re: [PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1957 bytes --]

Hi Arnd,

On Wed, 27 May 2020 11:26:51 +0200 Arnd Bergmann <arnd@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.148700-1-elver@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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH -tip v3 05/11] kcsan: Remove 'noinline' from __no_kcsan_or_inline

[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)) {

Re: [PATCH -tip v3 05/11] kcsan: Remove 'noinline' from __no_kcsan_or_inline

[Marco Elver]

On Fri, 29 May 2020 at 19:08, Peter Zijlstra <peterz@infradead.org> wrote:
[...]
>
> Doesn't this mean we can do the below?

If nobody complains about the lack of __no_kcsan_or_inline, let's do
it. See comments below.

> ---
>  Documentation/dev-tools/kcsan.rst |  6 ------
>  arch/x86/include/asm/bitops.h     |  6 +-----
>  include/linux/compiler_types.h    | 14 ++++----------
>  kernel/kcsan/kcsan-test.c         |  4 ++--
>  4 files changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/Documentation/dev-tools/kcsan.rst b/Documentation/dev-tools/kcsan.rst
> index ce4bbd918648..b38379f06194 100644
> --- a/Documentation/dev-tools/kcsan.rst
> +++ b/Documentation/dev-tools/kcsan.rst
> @@ -114,12 +114,6 @@ functions, compilation units, or entire subsystems.  For static blacklisting,
>    To dynamically limit for which functions to generate reports, see the
>    `DebugFS interface`_ blacklist/whitelist feature.
>
> -  For ``__always_inline`` functions, replace ``__always_inline`` with
> -  ``__no_kcsan_or_inline`` (which implies ``__always_inline``)::
> -
> -    static __no_kcsan_or_inline void foo(void) {
> -        ...
> -
>  * To disable data race detection for a particular compilation unit, add to the
>    ``Makefile``::

I suppose, if we say that __no_kcsan_or_inline should just disappear
because '__no_kcsan inline' is now good enough, we can delete it.

I think functions that absolutely must be __always_inline would break
with __no_kcsan_or_inline under KCSAN anyway. So, let's simplify.

> diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
> index 35460fef39b8..0367efdc5b7a 100644
> --- a/arch/x86/include/asm/bitops.h
> +++ b/arch/x86/include/asm/bitops.h
> @@ -201,12 +201,8 @@ arch_test_and_change_bit(long nr, volatile unsigned long *addr)
>         return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btc), *addr, c, "Ir", nr);
>  }
>
> -static __no_kcsan_or_inline bool constant_test_bit(long nr, const volatile unsigned long *addr)
> +static __always_inline bool constant_test_bit(long nr, const volatile unsigned long *addr)
>  {
> -       /*
> -        * Because this is a plain access, we need to disable KCSAN here to
> -        * avoid double instrumentation via instrumented bitops.
> -        */

Yes, we should have reverted this eventually.

>         return ((1UL << (nr & (BITS_PER_LONG-1))) &
>                 (addr[nr >> _BITOPS_LONG_SHIFT])) != 0;
>  }
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 4e4982d6f3b0..6a2c0f857ac3 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -118,10 +118,6 @@ struct ftrace_likely_data {
>  #define notrace                        __attribute__((__no_instrument_function__))
>  #endif
>
> -/* Section for code which can't be instrumented at all */
> -#define noinstr                                                                \
> -       noinline notrace __attribute((__section__(".noinstr.text")))
> -
>  /*
>   * it doesn't make sense on ARM (currently the only user of __naked)
>   * to trace naked functions because then mcount is called without
> @@ -192,17 +188,15 @@ struct ftrace_likely_data {
>  #endif
>
>  #define __no_kcsan __no_sanitize_thread
> -#ifdef __SANITIZE_THREAD__
> -# define __no_kcsan_or_inline __no_kcsan notrace __maybe_unused
> -# define __no_sanitize_or_inline __no_kcsan_or_inline

I think we just want to keep __no_sanitize_or_inline, for
READ_ONCE_NOCHECK. Having READ_ONCE_NOCHECK do KCSAN-checking seems
wrong, and I don't know what might break.

> -#else
> -# define __no_kcsan_or_inline __always_inline
> -#endif
>
>  #ifndef __no_sanitize_or_inline
>  #define __no_sanitize_or_inline __always_inline
>  #endif
>
> +/* Section for code which can't be instrumented at all */
> +#define noinstr                                                                \
> +       noinline notrace __attribute((__section__(".noinstr.text"))) __no_kcsan
> +

Will this eventually need __no_sanitize_address?

>  #endif /* __KERNEL__ */
>
>  #endif /* __ASSEMBLY__ */
> diff --git a/kernel/kcsan/kcsan-test.c b/kernel/kcsan/kcsan-test.c
> index a8c11506dd2a..374263ddffe2 100644
> --- a/kernel/kcsan/kcsan-test.c
> +++ b/kernel/kcsan/kcsan-test.c
> @@ -43,7 +43,7 @@ static struct {
>  };
>
>  /* Setup test checking loop. */
> -static __no_kcsan_or_inline void
> +static __no_kcsan inline void
>  begin_test_checks(void (*func1)(void), void (*func2)(void))
>  {
>         kcsan_disable_current();
> @@ -60,7 +60,7 @@ begin_test_checks(void (*func1)(void), void (*func2)(void))
>  }
>
>  /* End test checking loop. */
> -static __no_kcsan_or_inline bool
> +static __no_kcsan inline bool
>  end_test_checks(bool stop)
>  {
>         if (!stop && time_before(jiffies, end_time)) {

Acked -- if you send a patch, do split the test-related change, so
that Paul can apply it to the test which is currently only in -rcu.

Thanks,
-- Marco

Re: [PATCH -tip v3 05/11] kcsan: Remove 'noinline' from __no_kcsan_or_inline

[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/20200529171104.GD706518@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 ;-)