[PATCH 0/7] kcsan: Introduce CONFIG_KCSAN_PERMISSIVE

[PATCH 0/7] kcsan: Introduce CONFIG_KCSAN_PERMISSIVE

[Marco Elver]

While investigating a number of data races, we've encountered data-racy
accesses on flags variables to be very common. The typical pattern is a
reader masking all but one bit, and the writer setting/clearing only 1
bit (current->flags being a frequently encountered case; mm/sl[au]b.c
disables KCSAN for this reason currently).

Since these types of "trivial" data races are common (assuming they're
intentional and hard to miscompile!), having the option to filter them
(like we currently do for other types of data races) will avoid forcing
everyone to mark them, and deliberately left to preference at this time.

The primary motivation is to move closer towards more easily filtering
interesting data races (like [1], [2], [3]) on CI systems (e.g. syzbot),
without the churn to mark all such "trivial" data races.
[1] https://lkml.kernel.org/r/20210527092547.2656514-1-elver@google.com
[2] https://lkml.kernel.org/r/20210527104711.2671610-1-elver@google.com
[3] https://lkml.kernel.org/r/20210209112701.3341724-1-elver@google.com

Notably, the need for further built-in filtering has become clearer as
we notice some other CI systems (without active moderation) trying to
employ KCSAN, but usually have to turn it down quickly because their
reports are quickly met with negative feedback:
https://lkml.kernel.org/r/YHSPfiJ/h/f3ky5n@elver.google.com

The rules are implemented and guarded by a new option
CONFIG_KCSAN_PERMISSIVE. With it, we will ignore data races with only
1-bit value changes. Please see more details in in patch 7/7.

The rest of the patches are cleanups and improving configuration.

I ran some experiments to see what data races we're left with. With
CONFIG_KCSAN_PERMISSIVE=y paired with syzbot's current KCSAN config
(minimal kernel, most permissive KCSAN options), we're "just" about ~100
reports away to a pretty silent KCSAN kernel:

  https://github.com/google/ktsan/tree/kcsan-permissive-with-dataraces
  [ !!Disclaimer!! None of the commits are usable patches nor guaranteed
    to be correct -- they merely resolve a data race so it wouldn't be
    shown again and then moved on. Expect that simply marking is not
    enough for some! ]

Most of the data races look interesting enough, and only few already had
a comment nearby explaining what's happening.

All data races on current->flags, and most other flags are absent
(unlike before). Those that were reported all had value changes with >1
bit. A limitation is that few data races are still reported where the
reader is only interested in 1 bit but the writer changed more than 1
bit. A complete approach would require compiler changes in addition to
the changes in this series -- but since that would further reduce the
data races reported, the simpler and conservative approach is to stick
to the value-change based rules for now.

Marco Elver (7):
  kcsan: Improve some Kconfig comments
  kcsan: Remove CONFIG_KCSAN_DEBUG
  kcsan: Introduce CONFIG_KCSAN_STRICT
  kcsan: Reduce get_ctx() uses in kcsan_found_watchpoint()
  kcsan: Rework atomic.h into permissive.h
  kcsan: Print if strict or non-strict during init
  kcsan: permissive: Ignore data-racy 1-bit value changes

 Documentation/dev-tools/kcsan.rst | 12 ++++
 kernel/kcsan/atomic.h             | 23 --------
 kernel/kcsan/core.c               | 77 ++++++++++++++++---------
 kernel/kcsan/kcsan_test.c         | 32 +++++++++++
 kernel/kcsan/permissive.h         | 94 +++++++++++++++++++++++++++++++
 lib/Kconfig.kcsan                 | 39 +++++++++----
 6 files changed, 215 insertions(+), 62 deletions(-)
 delete mode 100644 kernel/kcsan/atomic.h
 create mode 100644 kernel/kcsan/permissive.h

-- 
2.32.0.rc1.229.g3e70b5a671-goog

[PATCH 1/7] kcsan: Improve some Kconfig comments

[Marco Elver]

Improve comment for CC_HAS_TSAN_COMPOUND_READ_BEFORE_WRITE. Also shorten
the comment above the "strictness" configuration options.

Signed-off-by: Marco Elver <elver@google.com>
---
 lib/Kconfig.kcsan | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index 0440f373248e..6152fbd5cbb4 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -40,10 +40,14 @@ menuconfig KCSAN
 
 if KCSAN
 
-# Compiler capabilities that should not fail the test if they are unavailable.
 config CC_HAS_TSAN_COMPOUND_READ_BEFORE_WRITE
 	def_bool (CC_IS_CLANG && $(cc-option,-fsanitize=thread -mllvm -tsan-compound-read-before-write=1)) || \
 		 (CC_IS_GCC && $(cc-option,-fsanitize=thread --param tsan-compound-read-before-write=1))
+	help
+	  The compiler instruments plain compound read-write operations
+	  differently (++, --, +=, -=, |=, &=, etc.), which allows KCSAN to
+	  distinguish them from other plain accesses. This is currently
+	  supported by Clang 12 or later.
 
 config KCSAN_VERBOSE
 	bool "Show verbose reports with more information about system state"
@@ -169,13 +173,9 @@ config KCSAN_REPORT_ONCE_IN_MS
 	  reporting to avoid flooding the console with reports.  Setting this
 	  to 0 disables rate limiting.
 
-# The main purpose of the below options is to control reported data races (e.g.
-# in fuzzer configs), and are not expected to be switched frequently by other
-# users. We could turn some of them into boot parameters, but given they should
-# not be switched normally, let's keep them here to simplify configuration.
-#
-# The defaults below are chosen to be very conservative, and may miss certain
-# bugs.
+# The main purpose of the below options is to control reported data races, and
+# are not expected to be switched frequently by non-testers or at runtime.
+# The defaults are chosen to be conservative, and can miss certain bugs.
 
 config KCSAN_REPORT_RACE_UNKNOWN_ORIGIN
 	bool "Report races of unknown origin"
-- 
2.32.0.rc1.229.g3e70b5a671-goog

[PATCH 2/7] kcsan: Remove CONFIG_KCSAN_DEBUG

[Marco Elver]

By this point CONFIG_KCSAN_DEBUG is pretty useless, as the system just
isn't usable with it due to spamming console (I imagine a randconfig
test robot will run into this sooner or later). Remove it.

Back in 2019 I used it occasionally to record traces of watchpoints and
verify the encoding is correct, but these days we have proper tests. If
something similar is needed in future, just add it back ad-hoc.

Signed-off-by: Marco Elver <elver@google.com>
---
 kernel/kcsan/core.c | 9 ---------
 lib/Kconfig.kcsan   | 3 ---
 2 files changed, 12 deletions(-)

diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c
index 26709ea65c71..d92977ede7e1 100644
--- a/kernel/kcsan/core.c
+++ b/kernel/kcsan/core.c
@@ -479,15 +479,6 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type)
 		break; /* ignore; we do not diff the values */
 	}
 
-	if (IS_ENABLED(CONFIG_KCSAN_DEBUG)) {
-		kcsan_disable_current();
-		pr_err("watching %s, size: %zu, addr: %px [slot: %d, encoded: %lx]\n",
-		       is_write ? "write" : "read", size, ptr,
-		       watchpoint_slot((unsigned long)ptr),
-		       encode_watchpoint((unsigned long)ptr, size, is_write));
-		kcsan_enable_current();
-	}
-
 	/*
 	 * Delay this thread, to increase probability of observing a racy
 	 * conflicting access.
diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index 6152fbd5cbb4..5304f211f81f 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -62,9 +62,6 @@ config KCSAN_VERBOSE
 	  generated from any one of them, system stability may suffer due to
 	  deadlocks or recursion.  If in doubt, say N.
 
-config KCSAN_DEBUG
-	bool "Debugging of KCSAN internals"
-
 config KCSAN_SELFTEST
 	bool "Perform short selftests on boot"
 	default y
-- 
2.32.0.rc1.229.g3e70b5a671-goog

[PATCH 3/7] kcsan: Introduce CONFIG_KCSAN_STRICT

[Marco Elver]

Add a simpler Kconfig variable to configure KCSAN's "strict" mode. This
makes it simpler in documentation or messages to suggest just a single
configuration option to select the strictest checking mode (vs.
currently having to list several options).

Signed-off-by: Marco Elver <elver@google.com>
---
 Documentation/dev-tools/kcsan.rst |  4 ++++
 lib/Kconfig.kcsan                 | 10 ++++++++++
 2 files changed, 14 insertions(+)

diff --git a/Documentation/dev-tools/kcsan.rst b/Documentation/dev-tools/kcsan.rst
index ba059df10b7d..17f974213b88 100644
--- a/Documentation/dev-tools/kcsan.rst
+++ b/Documentation/dev-tools/kcsan.rst
@@ -124,6 +124,10 @@ Kconfig options:
   causes KCSAN to not report data races due to conflicts where the only plain
   accesses are aligned writes up to word size.
 
+To use the strictest possible rules, select ``CONFIG_KCSAN_STRICT=y``, which
+configures KCSAN to follow the Linux-kernel memory consistency model (LKMM) as
+closely as possible.
+
 DebugFS interface
 ~~~~~~~~~~~~~~~~~
 
diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index 5304f211f81f..c76fbb3ee09e 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -183,9 +183,17 @@ config KCSAN_REPORT_RACE_UNKNOWN_ORIGIN
 	  reported if it was only possible to infer a race due to a data value
 	  change while an access is being delayed on a watchpoint.
 
+config KCSAN_STRICT
+	bool "Strict data-race checking"
+	help
+	  KCSAN will report data races with the strictest possible rules, which
+	  closely aligns with the rules defined by the Linux-kernel memory
+	  consistency model (LKMM).
+
 config KCSAN_REPORT_VALUE_CHANGE_ONLY
 	bool "Only report races where watcher observed a data value change"
 	default y
+	depends on !KCSAN_STRICT
 	help
 	  If enabled and a conflicting write is observed via a watchpoint, but
 	  the data value of the memory location was observed to remain
@@ -194,6 +202,7 @@ config KCSAN_REPORT_VALUE_CHANGE_ONLY
 config KCSAN_ASSUME_PLAIN_WRITES_ATOMIC
 	bool "Assume that plain aligned writes up to word size are atomic"
 	default y
+	depends on !KCSAN_STRICT
 	help
 	  Assume that plain aligned writes up to word size are atomic by
 	  default, and also not subject to other unsafe compiler optimizations
@@ -206,6 +215,7 @@ config KCSAN_ASSUME_PLAIN_WRITES_ATOMIC
 
 config KCSAN_IGNORE_ATOMICS
 	bool "Do not instrument marked atomic accesses"
+	depends on !KCSAN_STRICT
 	help
 	  Never instrument marked atomic accesses. This option can be used for
 	  additional filtering. Conflicting marked atomic reads and plain
-- 
2.32.0.rc1.229.g3e70b5a671-goog

[PATCH 4/7] kcsan: Reduce get_ctx() uses in kcsan_found_watchpoint()

[Marco Elver]

There are a number get_ctx() calls that are close to each other, which
results in poor codegen (repeated preempt_count loads).

Specifically in kcsan_found_watchpoint() (even though it's a slow-path)
it is beneficial to keep the race-window small until the watchpoint has
actually been consumed to avoid missed opportunities to report a race.

Let's clean it up a bit before we add more code in
kcsan_found_watchpoint().

Signed-off-by: Marco Elver <elver@google.com>
---
 kernel/kcsan/core.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c
index d92977ede7e1..906100923b88 100644
--- a/kernel/kcsan/core.c
+++ b/kernel/kcsan/core.c
@@ -301,9 +301,9 @@ static inline void reset_kcsan_skip(void)
 	this_cpu_write(kcsan_skip, skip_count);
 }
 
-static __always_inline bool kcsan_is_enabled(void)
+static __always_inline bool kcsan_is_enabled(struct kcsan_ctx *ctx)
 {
-	return READ_ONCE(kcsan_enabled) && get_ctx()->disable_count == 0;
+	return READ_ONCE(kcsan_enabled) && !ctx->disable_count;
 }
 
 /* Introduce delay depending on context and configuration. */
@@ -353,10 +353,17 @@ static noinline void kcsan_found_watchpoint(const volatile void *ptr,
 					    atomic_long_t *watchpoint,
 					    long encoded_watchpoint)
 {
+	struct kcsan_ctx *ctx = get_ctx();
 	unsigned long flags;
 	bool consumed;
 
-	if (!kcsan_is_enabled())
+	/*
+	 * We know a watchpoint exists. Let's try to keep the race-window
+	 * between here and finally consuming the watchpoint below as small as
+	 * possible -- avoid unneccessarily complex code until consumed.
+	 */
+
+	if (!kcsan_is_enabled(ctx))
 		return;
 
 	/*
@@ -364,14 +371,12 @@ static noinline void kcsan_found_watchpoint(const volatile void *ptr,
 	 * reporting a race where e.g. the writer set up the watchpoint, but the
 	 * reader has access_mask!=0, we have to ignore the found watchpoint.
 	 */
-	if (get_ctx()->access_mask != 0)
+	if (ctx->access_mask)
 		return;
 
 	/*
-	 * Consume the watchpoint as soon as possible, to minimize the chances
-	 * of !consumed. Consuming the watchpoint must always be guarded by
-	 * kcsan_is_enabled() check, as otherwise we might erroneously
-	 * triggering reports when disabled.
+	 * Consuming the watchpoint must be guarded by kcsan_is_enabled() to
+	 * avoid erroneously triggering reports if the context is disabled.
 	 */
 	consumed = try_consume_watchpoint(watchpoint, encoded_watchpoint);
 
@@ -409,6 +414,7 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type)
 	unsigned long access_mask;
 	enum kcsan_value_change value_change = KCSAN_VALUE_CHANGE_MAYBE;
 	unsigned long ua_flags = user_access_save();
+	struct kcsan_ctx *ctx = get_ctx();
 	unsigned long irq_flags = 0;
 
 	/*
@@ -417,7 +423,7 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type)
 	 */
 	reset_kcsan_skip();
 
-	if (!kcsan_is_enabled())
+	if (!kcsan_is_enabled(ctx))
 		goto out;
 
 	/*
@@ -489,7 +495,7 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type)
 	 * Re-read value, and check if it is as expected; if not, we infer a
 	 * racy access.
 	 */
-	access_mask = get_ctx()->access_mask;
+	access_mask = ctx->access_mask;
 	new = 0;
 	switch (size) {
 	case 1:
-- 
2.32.0.rc1.229.g3e70b5a671-goog

[PATCH 5/7] kcsan: Rework atomic.h into permissive.h

[Marco Elver]

Rework atomic.h into permissive.h to better reflect its purpose, and
introduce kcsan_ignore_address() and kcsan_ignore_data_race().

Introduce CONFIG_KCSAN_PERMISSIVE and update the stub functions in
preparation for subsequent changes.

As before, developers who choose to use KCSAN in "strict" mode will see
all data races and are not affected. Furthermore, by relying on the
value-change filter logic for kcsan_ignore_data_race(), even if the
permissive rules are enabled, the opt-outs in report.c:skip_report()
override them (such as for RCU-related functions by default).

The option CONFIG_KCSAN_PERMISSIVE is disabled by default, so that the
documented default behaviour of KCSAN does not change. Instead, like
CONFIG_KCSAN_IGNORE_ATOMICS, the option needs to be explicitly opted in.

Signed-off-by: Marco Elver <elver@google.com>
---
 Documentation/dev-tools/kcsan.rst |  8 ++++++
 kernel/kcsan/atomic.h             | 23 ---------------
 kernel/kcsan/core.c               | 33 ++++++++++++++++------
 kernel/kcsan/permissive.h         | 47 +++++++++++++++++++++++++++++++
 lib/Kconfig.kcsan                 | 10 +++++++
 5 files changed, 89 insertions(+), 32 deletions(-)
 delete mode 100644 kernel/kcsan/atomic.h
 create mode 100644 kernel/kcsan/permissive.h

diff --git a/Documentation/dev-tools/kcsan.rst b/Documentation/dev-tools/kcsan.rst
index 17f974213b88..9df98a48e69d 100644
--- a/Documentation/dev-tools/kcsan.rst
+++ b/Documentation/dev-tools/kcsan.rst
@@ -124,6 +124,14 @@ Kconfig options:
   causes KCSAN to not report data races due to conflicts where the only plain
   accesses are aligned writes up to word size.
 
+* ``CONFIG_KCSAN_PERMISSIVE``: Enable additional permissive rules to ignore
+  certain classes of common data races. Unlike the above, the rules are more
+  complex involving value-change patterns, access type, and address. This
+  option depends on ``CONFIG_KCSAN_REPORT_VALUE_CHANGE_ONLY=y``. For details
+  please see the ``kernel/kcsan/permissive.h``. Testers and maintainers that
+  only focus on reports from specific subsystems and not the whole kernel are
+  recommended to disable this option.
+
 To use the strictest possible rules, select ``CONFIG_KCSAN_STRICT=y``, which
 configures KCSAN to follow the Linux-kernel memory consistency model (LKMM) as
 closely as possible.
diff --git a/kernel/kcsan/atomic.h b/kernel/kcsan/atomic.h
deleted file mode 100644
index 530ae1bda8e7..000000000000
--- a/kernel/kcsan/atomic.h
+++ /dev/null
@@ -1,23 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Rules for implicitly atomic memory accesses.
- *
- * Copyright (C) 2019, Google LLC.
- */
-
-#ifndef _KERNEL_KCSAN_ATOMIC_H
-#define _KERNEL_KCSAN_ATOMIC_H
-
-#include <linux/types.h>
-
-/*
- * Special rules for certain memory where concurrent conflicting accesses are
- * common, however, the current convention is to not mark them; returns true if
- * access to @ptr should be considered atomic. Called from slow-path.
- */
-static bool kcsan_is_atomic_special(const volatile void *ptr)
-{
-	return false;
-}
-
-#endif /* _KERNEL_KCSAN_ATOMIC_H */
diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c
index 906100923b88..439edb9dcbb1 100644
--- a/kernel/kcsan/core.c
+++ b/kernel/kcsan/core.c
@@ -20,9 +20,9 @@
 #include <linux/sched.h>
 #include <linux/uaccess.h>
 
-#include "atomic.h"
 #include "encoding.h"
 #include "kcsan.h"
+#include "permissive.h"
 
 static bool kcsan_early_enable = IS_ENABLED(CONFIG_KCSAN_EARLY_ENABLE);
 unsigned int kcsan_udelay_task = CONFIG_KCSAN_UDELAY_TASK;
@@ -353,6 +353,7 @@ static noinline void kcsan_found_watchpoint(const volatile void *ptr,
 					    atomic_long_t *watchpoint,
 					    long encoded_watchpoint)
 {
+	const bool is_assert = (type & KCSAN_ACCESS_ASSERT) != 0;
 	struct kcsan_ctx *ctx = get_ctx();
 	unsigned long flags;
 	bool consumed;
@@ -374,6 +375,16 @@ static noinline void kcsan_found_watchpoint(const volatile void *ptr,
 	if (ctx->access_mask)
 		return;
 
+	/*
+	 * If the other thread does not want to ignore the access, and there was
+	 * a value change as a result of this thread's operation, we will still
+	 * generate a report of unknown origin.
+	 *
+	 * Use CONFIG_KCSAN_REPORT_RACE_UNKNOWN_ORIGIN=n to filter.
+	 */
+	if (!is_assert && kcsan_ignore_address(ptr))
+		return;
+
 	/*
 	 * Consuming the watchpoint must be guarded by kcsan_is_enabled() to
 	 * avoid erroneously triggering reports if the context is disabled.
@@ -396,7 +407,7 @@ static noinline void kcsan_found_watchpoint(const volatile void *ptr,
 		atomic_long_inc(&kcsan_counters[KCSAN_COUNTER_REPORT_RACES]);
 	}
 
-	if ((type & KCSAN_ACCESS_ASSERT) != 0)
+	if (is_assert)
 		atomic_long_inc(&kcsan_counters[KCSAN_COUNTER_ASSERT_FAILURES]);
 	else
 		atomic_long_inc(&kcsan_counters[KCSAN_COUNTER_DATA_RACES]);
@@ -427,12 +438,10 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type)
 		goto out;
 
 	/*
-	 * Special atomic rules: unlikely to be true, so we check them here in
-	 * the slow-path, and not in the fast-path in is_atomic(). Call after
-	 * kcsan_is_enabled(), as we may access memory that is not yet
-	 * initialized during early boot.
+	 * Check to-ignore addresses after kcsan_is_enabled(), as we may access
+	 * memory that is not yet initialized during early boot.
 	 */
-	if (!is_assert && kcsan_is_atomic_special(ptr))
+	if (!is_assert && kcsan_ignore_address(ptr))
 		goto out;
 
 	if (!check_encodable((unsigned long)ptr, size)) {
@@ -518,8 +527,14 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type)
 	if (access_mask)
 		diff &= access_mask;
 
-	/* Were we able to observe a value-change? */
-	if (diff != 0)
+	/*
+	 * Check if we observed a value change.
+	 *
+	 * Also check if the data race should be ignored (the rules depend on
+	 * non-zero diff); if it is to be ignored, the below rules for
+	 * KCSAN_VALUE_CHANGE_MAYBE apply.
+	 */
+	if (diff && !kcsan_ignore_data_race(size, type, old, new, diff))
 		value_change = KCSAN_VALUE_CHANGE_TRUE;
 
 	/* Check if this access raced with another. */
diff --git a/kernel/kcsan/permissive.h b/kernel/kcsan/permissive.h
new file mode 100644
index 000000000000..f90e30800c11
--- /dev/null
+++ b/kernel/kcsan/permissive.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Special rules for ignoring entire classes of data-racy memory accesses. None
+ * of the rules here imply that such data races are generally safe!
+ *
+ * All rules in this file can be configured via CONFIG_KCSAN_PERMISSIVE. Keep
+ * them separate from core code to make it easier to audit.
+ *
+ * Copyright (C) 2019, Google LLC.
+ */
+
+#ifndef _KERNEL_KCSAN_PERMISSIVE_H
+#define _KERNEL_KCSAN_PERMISSIVE_H
+
+#include <linux/types.h>
+
+/*
+ * Access ignore rules based on address.
+ */
+static __always_inline bool kcsan_ignore_address(const volatile void *ptr)
+{
+	if (!IS_ENABLED(CONFIG_KCSAN_PERMISSIVE))
+		return false;
+
+	return false;
+}
+
+/*
+ * Data race ignore rules based on access type and value change patterns.
+ */
+static bool
+kcsan_ignore_data_race(size_t size, int type, u64 old, u64 new, u64 diff)
+{
+	if (!IS_ENABLED(CONFIG_KCSAN_PERMISSIVE))
+		return false;
+
+	/*
+	 * Rules here are only for plain read accesses, so that we still report
+	 * data races between plain read-write accesses.
+	 */
+	if (type || size > sizeof(long))
+		return false;
+
+	return false;
+}
+
+#endif /* _KERNEL_KCSAN_PERMISSIVE_H */
diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index c76fbb3ee09e..26f03c754d39 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -231,4 +231,14 @@ config KCSAN_IGNORE_ATOMICS
 	  due to two conflicting plain writes will be reported (aligned and
 	  unaligned, if CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=n).
 
+config KCSAN_PERMISSIVE
+	bool "Enable all additional permissive rules"
+	depends on KCSAN_REPORT_VALUE_CHANGE_ONLY
+	help
+	  Enable additional permissive rules to ignore certain classes of data
+	  races (also see kernel/kcsan/permissive.h). None of the permissive
+	  rules imply that such data races are generally safe, but can be used
+	  to further reduce reported data races due to data-racy patterns
+	  common across the kernel.
+
 endif # KCSAN
-- 
2.32.0.rc1.229.g3e70b5a671-goog

[PATCH 6/7] kcsan: Print if strict or non-strict during init

[Marco Elver]

Show a brief message if KCSAN is strict or non-strict, and if non-strict
also say that CONFIG_KCSAN_STRICT=y can be used to see all data races.

This is to hint to users of KCSAN who blindly use the default config
that their configuration might miss data races of interest.

Signed-off-by: Marco Elver <elver@google.com>
---
 kernel/kcsan/core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c
index 439edb9dcbb1..76e67d1e02d4 100644
--- a/kernel/kcsan/core.c
+++ b/kernel/kcsan/core.c
@@ -656,6 +656,15 @@ void __init kcsan_init(void)
 		pr_info("enabled early\n");
 		WRITE_ONCE(kcsan_enabled, true);
 	}
+
+	if (IS_ENABLED(CONFIG_KCSAN_REPORT_VALUE_CHANGE_ONLY) ||
+	    IS_ENABLED(CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC) ||
+	    IS_ENABLED(CONFIG_KCSAN_PERMISSIVE) ||
+	    IS_ENABLED(CONFIG_KCSAN_IGNORE_ATOMICS)) {
+		pr_warn("non-strict mode configured - use CONFIG_KCSAN_STRICT=y to see all data races\n");
+	} else {
+		pr_info("strict mode configured\n");
+	}
 }
 
 /* === Exported interface =================================================== */
-- 
2.32.0.rc1.229.g3e70b5a671-goog

[PATCH 7/7] kcsan: permissive: Ignore data-racy 1-bit value changes

[Marco Elver]

Add rules to ignore data-racy reads with only 1-bit value changes.
Details about the rules are captured in comments in
kernel/kcsan/permissive.h. More background follows.

While investigating a number of data races, we've encountered data-racy
accesses on flags variables to be very common. The typical pattern is a
reader masking all but one bit, and/or the writer setting/clearing only
1 bit (current->flags being a frequently encountered case; more examples
in mm/sl[au]b.c, which disable KCSAN for this reason).

Since these types of data-racy accesses are common (with the assumption
they are intentional and hard to miscompile) having the option (with
CONFIG_KCSAN_PERMISSIVE=y) to filter them will avoid forcing everyone to
mark them, and deliberately left to preference at this time.

One important motivation for having this option built-in is to move
closer to being able to enable KCSAN on CI systems or for testers
wishing to test the whole kernel, while more easily filtering
less interesting data races with higher probability.

For the implementation, we considered several alternatives, but had one
major requirement: that the rules be kept together with the Linux-kernel
tree. Adding them to the compiler would preclude us from making changes
quickly; if the rules require tweaks, having them part of the compiler
requires waiting another ~1 year for the next release -- that's not
realistic. We are left with the following options:

	1. Maintain compiler plugins as part of the kernel-tree that
	   removes instrumentation for some accesses (e.g. plain-& with
	   1-bit mask). The analysis would be reader-side focused, as
	   no assumption can be made about racing writers.

Because it seems unrealistic to maintain 2 plugins, one for LLVM and
GCC, we would likely pick LLVM. Furthermore, no kernel infrastructure
exists to maintain LLVM plugins, and the build-system implications and
maintenance overheads do not look great (historically, plugins written
against old LLVM APIs are not guaranteed to work with newer LLVM APIs).

	2. Find a set of rules that can be expressed in terms of
	   observed value changes, and make it part of the KCSAN runtime.
	   The analysis is writer-side focused, given we rely on observed
	   value changes.

The approach taken here is (2). While a complete approach requires both
(1) and (2), experiments show that the majority of data races involving
trivial bit operations on flags variables can be removed with (2) alone.

It goes without saying that the filtering of data races using (1) or (2)
does _not_ guarantee they are safe! Therefore, limiting ourselves to (2)
for now is the conservative choice for setups that wish to enable
CONFIG_KCSAN_PERMISSIVE=y.

Signed-off-by: Marco Elver <elver@google.com>
---
 kernel/kcsan/kcsan_test.c | 32 +++++++++++++++++++++++++
 kernel/kcsan/permissive.h | 49 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/kernel/kcsan/kcsan_test.c b/kernel/kcsan/kcsan_test.c
index 8bcffbdef3d3..dc55fd5a36fc 100644
--- a/kernel/kcsan/kcsan_test.c
+++ b/kernel/kcsan/kcsan_test.c
@@ -414,6 +414,14 @@ static noinline void test_kernel_atomic_builtins(void)
 	__atomic_load_n(&test_var, __ATOMIC_RELAXED);
 }
 
+static noinline void test_kernel_xor_1bit(void)
+{
+	/* Do not report data races between the read-writes. */
+	kcsan_nestable_atomic_begin();
+	test_var ^= 0x10000;
+	kcsan_nestable_atomic_end();
+}
+
 /* ===== Test cases ===== */
 
 /* Simple test with normal data race. */
@@ -952,6 +960,29 @@ static void test_atomic_builtins(struct kunit *test)
 	KUNIT_EXPECT_FALSE(test, match_never);
 }
 
+__no_kcsan
+static void test_1bit_value_change(struct kunit *test)
+{
+	const struct expect_report expect = {
+		.access = {
+			{ test_kernel_read, &test_var, sizeof(test_var), 0 },
+			{ test_kernel_xor_1bit, &test_var, sizeof(test_var), __KCSAN_ACCESS_RW(KCSAN_ACCESS_WRITE) },
+		},
+	};
+	bool match = false;
+
+	begin_test_checks(test_kernel_read, test_kernel_xor_1bit);
+	do {
+		match = IS_ENABLED(CONFIG_KCSAN_PERMISSIVE)
+				? report_available()
+				: report_matches(&expect);
+	} while (!end_test_checks(match));
+	if (IS_ENABLED(CONFIG_KCSAN_PERMISSIVE))
+		KUNIT_EXPECT_FALSE(test, match);
+	else
+		KUNIT_EXPECT_TRUE(test, match);
+}
+
 /*
  * Generate thread counts for all test cases. Values generated are in interval
  * [2, 5] followed by exponentially increasing thread counts from 8 to 32.
@@ -1024,6 +1055,7 @@ static struct kunit_case kcsan_test_cases[] = {
 	KCSAN_KUNIT_CASE(test_jiffies_noreport),
 	KCSAN_KUNIT_CASE(test_seqlock_noreport),
 	KCSAN_KUNIT_CASE(test_atomic_builtins),
+	KCSAN_KUNIT_CASE(test_1bit_value_change),
 	{},
 };
 
diff --git a/kernel/kcsan/permissive.h b/kernel/kcsan/permissive.h
index f90e30800c11..2c01fe4a59ee 100644
--- a/kernel/kcsan/permissive.h
+++ b/kernel/kcsan/permissive.h
@@ -12,6 +12,8 @@
 #ifndef _KERNEL_KCSAN_PERMISSIVE_H
 #define _KERNEL_KCSAN_PERMISSIVE_H
 
+#include <linux/bitops.h>
+#include <linux/sched.h>
 #include <linux/types.h>
 
 /*
@@ -22,7 +24,11 @@ static __always_inline bool kcsan_ignore_address(const volatile void *ptr)
 	if (!IS_ENABLED(CONFIG_KCSAN_PERMISSIVE))
 		return false;
 
-	return false;
+	/*
+	 * Data-racy bitops on current->flags are too common, ignore completely
+	 * for now.
+	 */
+	return ptr == &current->flags;
 }
 
 /*
@@ -41,6 +47,47 @@ kcsan_ignore_data_race(size_t size, int type, u64 old, u64 new, u64 diff)
 	if (type || size > sizeof(long))
 		return false;
 
+	/*
+	 * A common pattern is checking/setting just 1 bit in a variable; for
+	 * example:
+	 *
+	 *	if (flags & SOME_FLAG) { ... }
+	 *
+	 * and elsewhere flags is updated concurrently:
+	 *
+	 *	flags |= SOME_OTHER_FLAG; // just 1 bit
+	 *
+	 * While it is still recommended that such accesses be marked
+	 * appropriately, in many cases these types of data races are so common
+	 * that marking them all is often unrealistic and left to maintainer
+	 * preference.
+	 *
+	 * The assumption in all cases is that with all known compiler
+	 * optimizations (including those that tear accesses), because no more
+	 * than 1 bit changed, the plain accesses are safe despite the presence
+	 * of data races.
+	 *
+	 * The rules here will ignore the data races if we observe no more than
+	 * 1 bit changed.
+	 *
+	 * Of course many operations can effecively change just 1 bit, but the
+	 * general assuption that data races involving 1-bit changes can be
+	 * tolerated still applies.
+	 *
+	 * And in case a true bug is missed, the bug likely manifests as a
+	 * reportable data race elsewhere.
+	 */
+	if (hweight64(diff) == 1) {
+		/*
+		 * Exception: Report data races where the values look like
+		 * ordinary booleans (one of them was 0 and the 0th bit was
+		 * changed) More often than not, they come with interesting
+		 * memory ordering requirements, so let's report them.
+		 */
+		if (!((!old || !new) && diff == 1))
+			return true;
+	}
+
 	return false;
 }
 
-- 
2.32.0.rc1.229.g3e70b5a671-goog

Re: [PATCH 0/7] kcsan: Introduce CONFIG_KCSAN_PERMISSIVE

[Mark Rutland]

Hi Marco,

On Mon, Jun 07, 2021 at 02:56:46PM +0200, Marco Elver wrote:
> While investigating a number of data races, we've encountered data-racy
> accesses on flags variables to be very common. The typical pattern is a
> reader masking all but one bit, and the writer setting/clearing only 1
> bit (current->flags being a frequently encountered case; mm/sl[au]b.c
> disables KCSAN for this reason currently).

As a heads up, I just sent out the series I promised for
thread_info::flags, at:

  https://lore.kernel.org/lkml/20210609122001.18277-1-mark.rutland@arm.com/T/#t

... which I think is complementary to this (IIUC it should help with the
multi-bit cases you mention below), and may help to make the checks more
stringent in future.

FWIW, for this series:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> Since these types of "trivial" data races are common (assuming they're
> intentional and hard to miscompile!), having the option to filter them
> (like we currently do for other types of data races) will avoid forcing
> everyone to mark them, and deliberately left to preference at this time.
> 
> The primary motivation is to move closer towards more easily filtering
> interesting data races (like [1], [2], [3]) on CI systems (e.g. syzbot),
> without the churn to mark all such "trivial" data races.
> [1] https://lkml.kernel.org/r/20210527092547.2656514-1-elver@google.com
> [2] https://lkml.kernel.org/r/20210527104711.2671610-1-elver@google.com
> [3] https://lkml.kernel.org/r/20210209112701.3341724-1-elver@google.com
> 
> Notably, the need for further built-in filtering has become clearer as
> we notice some other CI systems (without active moderation) trying to
> employ KCSAN, but usually have to turn it down quickly because their
> reports are quickly met with negative feedback:
> https://lkml.kernel.org/r/YHSPfiJ/h/f3ky5n@elver.google.com
> 
> The rules are implemented and guarded by a new option
> CONFIG_KCSAN_PERMISSIVE. With it, we will ignore data races with only
> 1-bit value changes. Please see more details in in patch 7/7.
> 
> The rest of the patches are cleanups and improving configuration.
> 
> I ran some experiments to see what data races we're left with. With
> CONFIG_KCSAN_PERMISSIVE=y paired with syzbot's current KCSAN config
> (minimal kernel, most permissive KCSAN options), we're "just" about ~100
> reports away to a pretty silent KCSAN kernel:
> 
>   https://github.com/google/ktsan/tree/kcsan-permissive-with-dataraces
>   [ !!Disclaimer!! None of the commits are usable patches nor guaranteed
>     to be correct -- they merely resolve a data race so it wouldn't be
>     shown again and then moved on. Expect that simply marking is not
>     enough for some! ]
> 
> Most of the data races look interesting enough, and only few already had
> a comment nearby explaining what's happening.
> 
> All data races on current->flags, and most other flags are absent
> (unlike before). Those that were reported all had value changes with >1
> bit. A limitation is that few data races are still reported where the
> reader is only interested in 1 bit but the writer changed more than 1
> bit. A complete approach would require compiler changes in addition to
> the changes in this series -- but since that would further reduce the
> data races reported, the simpler and conservative approach is to stick
> to the value-change based rules for now.
> 
> Marco Elver (7):
>   kcsan: Improve some Kconfig comments
>   kcsan: Remove CONFIG_KCSAN_DEBUG
>   kcsan: Introduce CONFIG_KCSAN_STRICT
>   kcsan: Reduce get_ctx() uses in kcsan_found_watchpoint()
>   kcsan: Rework atomic.h into permissive.h
>   kcsan: Print if strict or non-strict during init
>   kcsan: permissive: Ignore data-racy 1-bit value changes
> 
>  Documentation/dev-tools/kcsan.rst | 12 ++++
>  kernel/kcsan/atomic.h             | 23 --------
>  kernel/kcsan/core.c               | 77 ++++++++++++++++---------
>  kernel/kcsan/kcsan_test.c         | 32 +++++++++++
>  kernel/kcsan/permissive.h         | 94 +++++++++++++++++++++++++++++++
>  lib/Kconfig.kcsan                 | 39 +++++++++----
>  6 files changed, 215 insertions(+), 62 deletions(-)
>  delete mode 100644 kernel/kcsan/atomic.h
>  create mode 100644 kernel/kcsan/permissive.h
> 
> -- 
> 2.32.0.rc1.229.g3e70b5a671-goog
> 

Re: [PATCH 0/7] kcsan: Introduce CONFIG_KCSAN_PERMISSIVE

[Marco Elver]

On Wed, 9 Jun 2021 at 14:38, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Marco,
>
> On Mon, Jun 07, 2021 at 02:56:46PM +0200, Marco Elver wrote:
> > While investigating a number of data races, we've encountered data-racy
> > accesses on flags variables to be very common. The typical pattern is a
> > reader masking all but one bit, and the writer setting/clearing only 1
> > bit (current->flags being a frequently encountered case; mm/sl[au]b.c
> > disables KCSAN for this reason currently).
>
> As a heads up, I just sent out the series I promised for
> thread_info::flags, at:
>
>   https://lore.kernel.org/lkml/20210609122001.18277-1-mark.rutland@arm.com/T/#t
>
> ... which I think is complementary to this (IIUC it should help with the
> multi-bit cases you mention below), and may help to make the checks more
> stringent in future.

Nice, glad to see this.

And yes, this series isn't a permission to let the 'flags' variables
be forgotten, but perhaps not every subsystem wants to go through this
now. So seeing any progress on this front helps and we can also use it
to give concrete suggestions how to approach it (e.g. your accessors).

> FWIW, for this series:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Thank you!

Re: [PATCH 0/7] kcsan: Introduce CONFIG_KCSAN_PERMISSIVE

[Paul E. McKenney]

On Wed, Jun 09, 2021 at 01:38:10PM +0100, Mark Rutland wrote:
> Hi Marco,
> 
> On Mon, Jun 07, 2021 at 02:56:46PM +0200, Marco Elver wrote:
> > While investigating a number of data races, we've encountered data-racy
> > accesses on flags variables to be very common. The typical pattern is a
> > reader masking all but one bit, and the writer setting/clearing only 1
> > bit (current->flags being a frequently encountered case; mm/sl[au]b.c
> > disables KCSAN for this reason currently).
> 
> As a heads up, I just sent out the series I promised for
> thread_info::flags, at:
> 
>   https://lore.kernel.org/lkml/20210609122001.18277-1-mark.rutland@arm.com/T/#t
> 
> ... which I think is complementary to this (IIUC it should help with the
> multi-bit cases you mention below), and may help to make the checks more
> stringent in future.
> 
> FWIW, for this series:
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Queued and pushed for v5.15, thank you both!

I also queued the following patch making use of CONFIG_KCSAN_STRICT, and I
figured that I should run it past you guys to make check my understanding.

Thoughts?

							Thanx, Paul

------------------------------------------------------------------------

commit 023f1604e373575be6335f85abf36fd475d78da3
Author: Paul E. McKenney <paulmck@kernel.org>
Date:   Tue Jun 15 11:14:19 2021 -0700

    torture: Apply CONFIG_KCSAN_STRICT to kvm.sh --kcsan argument
    
    Currently, the --kcsan argument to kvm.sh applies a laundry list of
    Kconfig options.  Now that KCSAN provides the CONFIG_KCSAN_STRICT Kconfig
    option, this commit reduces the laundry list to this one option.
    
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/tools/testing/selftests/rcutorture/bin/kvm.sh b/tools/testing/selftests/rcutorture/bin/kvm.sh
index b4ac4ee33222..f2bd80391999 100755
--- a/tools/testing/selftests/rcutorture/bin/kvm.sh
+++ b/tools/testing/selftests/rcutorture/bin/kvm.sh
@@ -184,7 +184,7 @@ do
 		TORTURE_KCONFIG_KASAN_ARG="CONFIG_DEBUG_INFO=y CONFIG_KASAN=y"; export TORTURE_KCONFIG_KASAN_ARG
 		;;
 	--kcsan)
-		TORTURE_KCONFIG_KCSAN_ARG="CONFIG_DEBUG_INFO=y CONFIG_KCSAN=y CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=n CONFIG_KCSAN_REPORT_VALUE_CHANGE_ONLY=n CONFIG_KCSAN_REPORT_ONCE_IN_MS=100000 CONFIG_KCSAN_INTERRUPT_WATCHER=y CONFIG_KCSAN_VERBOSE=y CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y"; export TORTURE_KCONFIG_KCSAN_ARG
+		TORTURE_KCONFIG_KCSAN_ARG="CONFIG_DEBUG_INFO=y CONFIG_KCSAN=y CONFIG_KCSAN_STRICT=y CONFIG_KCSAN_REPORT_ONCE_IN_MS=100000 CONFIG_KCSAN_VERBOSE=y CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y"; export TORTURE_KCONFIG_KCSAN_ARG
 		;;
 	--kmake-arg|--kmake-args)
 		checkarg --kmake-arg "(kernel make arguments)" $# "$2" '.*' '^error$'

Re: [PATCH 0/7] kcsan: Introduce CONFIG_KCSAN_PERMISSIVE

[Marco Elver]

On Tue, Jun 15, 2021 at 11:19AM -0700, Paul E. McKenney wrote:
[...]
> Queued and pushed for v5.15, thank you both!
> 
> I also queued the following patch making use of CONFIG_KCSAN_STRICT, and I
> figured that I should run it past you guys to make check my understanding.
> 
> Thoughts?

You still need CONFIG_KCSAN_INTERRUPT_WATCHER=y, but otherwise looks
good.

I thought I'd leave that out for now, but now thinking about it, we
might as well imply interruptible watchers. If you agree, feel free to
queue the below patch ahead of yours.

Thanks,
-- Marco

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

From: Marco Elver <elver@google.com>
Date: Tue, 15 Jun 2021 20:39:38 +0200
Subject: [PATCH] kcsan: Make strict mode imply interruptible watchers

If CONFIG_KCSAN_STRICT=y, select CONFIG_KCSAN_INTERRUPT_WATCHER as well.

With interruptible watchers, we'll also report same-CPU data races; if
we requested strict mode, we might as well show these, too.

Suggested-by: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Marco Elver <elver@google.com>
---
 lib/Kconfig.kcsan | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index 26f03c754d39..e0a93ffdef30 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -150,7 +150,8 @@ config KCSAN_SKIP_WATCH_RANDOMIZE
 	  KCSAN_WATCH_SKIP.
 
 config KCSAN_INTERRUPT_WATCHER
-	bool "Interruptible watchers"
+	bool "Interruptible watchers" if !KCSAN_STRICT
+	default KCSAN_STRICT
 	help
 	  If enabled, a task that set up a watchpoint may be interrupted while
 	  delayed. This option will allow KCSAN to detect races between
-- 
2.32.0.272.g935e593368-goog

Re: [PATCH 0/7] kcsan: Introduce CONFIG_KCSAN_PERMISSIVE

[Paul E. McKenney]

On Tue, Jun 15, 2021 at 08:51:18PM +0200, Marco Elver wrote:
> On Tue, Jun 15, 2021 at 11:19AM -0700, Paul E. McKenney wrote:
> [...]
> > Queued and pushed for v5.15, thank you both!
> > 
> > I also queued the following patch making use of CONFIG_KCSAN_STRICT, and I
> > figured that I should run it past you guys to make check my understanding.
> > 
> > Thoughts?
> 
> You still need CONFIG_KCSAN_INTERRUPT_WATCHER=y, but otherwise looks
> good.

I knew I was missing something...  :-/

> I thought I'd leave that out for now, but now thinking about it, we
> might as well imply interruptible watchers. If you agree, feel free to
> queue the below patch ahead of yours.

That works for me!  I have queued the patch below and rebased it to
precede my change to the torture-test infrastructure.

							Thanx, Paul

> Thanks,
> -- Marco
> 
> ------ >8 ------
> 
> From: Marco Elver <elver@google.com>
> Date: Tue, 15 Jun 2021 20:39:38 +0200
> Subject: [PATCH] kcsan: Make strict mode imply interruptible watchers
> 
> If CONFIG_KCSAN_STRICT=y, select CONFIG_KCSAN_INTERRUPT_WATCHER as well.
> 
> With interruptible watchers, we'll also report same-CPU data races; if
> we requested strict mode, we might as well show these, too.
> 
> Suggested-by: Paul E. McKenney <paulmck@kernel.org>
> Signed-off-by: Marco Elver <elver@google.com>
> ---
>  lib/Kconfig.kcsan | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
> index 26f03c754d39..e0a93ffdef30 100644
> --- a/lib/Kconfig.kcsan
> +++ b/lib/Kconfig.kcsan
> @@ -150,7 +150,8 @@ config KCSAN_SKIP_WATCH_RANDOMIZE
>  	  KCSAN_WATCH_SKIP.
>  
>  config KCSAN_INTERRUPT_WATCHER
> -	bool "Interruptible watchers"
> +	bool "Interruptible watchers" if !KCSAN_STRICT
> +	default KCSAN_STRICT
>  	help
>  	  If enabled, a task that set up a watchpoint may be interrupted while
>  	  delayed. This option will allow KCSAN to detect races between
> -- 
> 2.32.0.272.g935e593368-goog
>