History

Merge branch 'kcsan' of git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu into locking/debug

Pull KCSAN updates from Paul E. McKenney: - improve comments - introduce CONFIG_KCSAN_STRICT (which RCU uses) - optimize use of get_ctx() by kcsan_found_watchpoint() - rework atomic.h into permissive.h - add the ability to ignore writes that change only one bit of a given data-racy variable.

Ingo Molnar committed 5 years ago

Commit 7f3b4572 parents 9ae6ab2 e0493806 files · +217 −63Browse files

Showing the changes to kernel (4 of 6 files). Show all

Unified · Split

4 files
  1. kernel/kcsan/atomic.h +0 −23
  2. kernel/kcsan/core.c +49 −28
  3. kernel/kcsan/kcsan_test.c +32 −0
  4. kernel/kcsan/permissive.h +94 −0
kernel/kcsan/atomic.h+0−23
@@ -1,23 +0,0 @@
1−/* SPDX-License-Identifier: GPL-2.0 */
2−/*
3− * Rules for implicitly atomic memory accesses.
4− *
5− * Copyright (C) 2019, Google LLC.
6− */
7−
8−#ifndef _KERNEL_KCSAN_ATOMIC_H
9−#define _KERNEL_KCSAN_ATOMIC_H
10−
11−#include <linux/types.h>
12−
13−/*
14− * Special rules for certain memory where concurrent conflicting accesses are
15− * common, however, the current convention is to not mark them; returns true if
16− * access to @ptr should be considered atomic. Called from slow-path.
17− */
18−static bool kcsan_is_atomic_special(const volatile void *ptr)
19−{
20− return false;
21−}
22−
23−#endif /* _KERNEL_KCSAN_ATOMIC_H */
kernel/kcsan/core.c+49−28
@@ -20,9 +20,9 @@ · 19 unmodified lines
2020 #include <linux/sched.h>
2121 #include <linux/uaccess.h>
2222
23−#include "atomic.h"
2423 #include "encoding.h"
2524 #include "kcsan.h"
25+#include "permissive.h"
2626
2727 static bool kcsan_early_enable = IS_ENABLED(CONFIG_KCSAN_EARLY_ENABLE);
2828 unsigned int kcsan_udelay_task = CONFIG_KCSAN_UDELAY_TASK;
@@ -301,9 +301,9 @@ static inline void reset_kcsan_skip(void) · 272 unmodified lines
301301 this_cpu_write(kcsan_skip, skip_count);
302302 }
303303
304−static __always_inline bool kcsan_is_enabled(void)
304+static __always_inline bool kcsan_is_enabled(struct kcsan_ctx *ctx)
305305 {
306− return READ_ONCE(kcsan_enabled) && get_ctx()->disable_count == 0;
306+ return READ_ONCE(kcsan_enabled) && !ctx->disable_count;
307307 }
308308
309309 /* Introduce delay depending on context and configuration. */
@@ -353,10 +353,18 @@ static noinline void kcsan_found_watchpoint(const volatile void *ptr, · 43 unmodified lines
353353 atomic_long_t *watchpoint,
354354 long encoded_watchpoint)
355355 {
356+ const bool is_assert = (type & KCSAN_ACCESS_ASSERT) != 0;
357+ struct kcsan_ctx *ctx = get_ctx();
356358 unsigned long flags;
357359 bool consumed;
358360
359− if (!kcsan_is_enabled())
361+ /*
362+ * We know a watchpoint exists. Let's try to keep the race-window
363+ * between here and finally consuming the watchpoint below as small as
364+ * possible -- avoid unneccessarily complex code until consumed.
365+ */
366+
367+ if (!kcsan_is_enabled(ctx))
360368 return;
361369
362370 /*
@@ -364,14 +372,22 @@ static noinline void kcsan_found_watchpoint(const volatile void *ptr, · 1 unmodified line
364372 * reporting a race where e.g. the writer set up the watchpoint, but the
365373 * reader has access_mask!=0, we have to ignore the found watchpoint.
366374 */
367− if (get_ctx()->access_mask != 0)
375+ if (ctx->access_mask)
368376 return;
369377
370378 /*
371− * Consume the watchpoint as soon as possible, to minimize the chances
372− * of !consumed. Consuming the watchpoint must always be guarded by
373− * kcsan_is_enabled() check, as otherwise we might erroneously
374− * triggering reports when disabled.
379+ * If the other thread does not want to ignore the access, and there was
380+ * a value change as a result of this thread's operation, we will still
381+ * generate a report of unknown origin.
382+ *
383+ * Use CONFIG_KCSAN_REPORT_RACE_UNKNOWN_ORIGIN=n to filter.
384+ */
385+ if (!is_assert && kcsan_ignore_address(ptr))
386+ return;
387+
388+ /*
389+ * Consuming the watchpoint must be guarded by kcsan_is_enabled() to
390+ * avoid erroneously triggering reports if the context is disabled.
375391 */
376392 consumed = try_consume_watchpoint(watchpoint, encoded_watchpoint);
377393
@@ -391,7 +407,7 @@ static noinline void kcsan_found_watchpoint(const volatile void *ptr, · 13 unmodified lines
391407 atomic_long_inc(&kcsan_counters[KCSAN_COUNTER_REPORT_RACES]);
392408 }
393409
394− if ((type & KCSAN_ACCESS_ASSERT) != 0)
410+ if (is_assert)
395411 atomic_long_inc(&kcsan_counters[KCSAN_COUNTER_ASSERT_FAILURES]);
396412 else
397413 atomic_long_inc(&kcsan_counters[KCSAN_COUNTER_DATA_RACES]);
@@ -409,6 +425,7 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type) · 11 unmodified lines
409425 unsigned long access_mask;
410426 enum kcsan_value_change value_change = KCSAN_VALUE_CHANGE_MAYBE;
411427 unsigned long ua_flags = user_access_save();
428+ struct kcsan_ctx *ctx = get_ctx();
412429 unsigned long irq_flags = 0;
413430
414431 /*
@@ -417,16 +434,14 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type) · 2 unmodified lines
417434 */
418435 reset_kcsan_skip();
419436
420− if (!kcsan_is_enabled())
437+ if (!kcsan_is_enabled(ctx))
421438 goto out;
422439
423440 /*
424− * Special atomic rules: unlikely to be true, so we check them here in
425− * the slow-path, and not in the fast-path in is_atomic(). Call after
426− * kcsan_is_enabled(), as we may access memory that is not yet
427− * initialized during early boot.
441+ * Check to-ignore addresses after kcsan_is_enabled(), as we may access
442+ * memory that is not yet initialized during early boot.
428443 */
429− if (!is_assert && kcsan_is_atomic_special(ptr))
444+ if (!is_assert && kcsan_ignore_address(ptr))
430445 goto out;
431446
432447 if (!check_encodable((unsigned long)ptr, size)) {
@@ -479,15 +494,6 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type) · 46 unmodified lines
479494 break; /* ignore; we do not diff the values */
480495 }
481496
482− if (IS_ENABLED(CONFIG_KCSAN_DEBUG)) {
483− kcsan_disable_current();
484− pr_err("watching %s, size: %zu, addr: %px [slot: %d, encoded: %lx]\n",
485− is_write ? "write" : "read", size, ptr,
486− watchpoint_slot((unsigned long)ptr),
487− encode_watchpoint((unsigned long)ptr, size, is_write));
488− kcsan_enable_current();
489− }
490−
491497 /*
492498 * Delay this thread, to increase probability of observing a racy
493499 * conflicting access.
@@ -498,7 +504,7 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type) · 4 unmodified lines
498504 * Re-read value, and check if it is as expected; if not, we infer a
499505 * racy access.
500506 */
501− access_mask = get_ctx()->access_mask;
507+ access_mask = ctx->access_mask;
502508 new = 0;
503509 switch (size) {
504510 case 1:
@@ -521,8 +527,14 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type) · 16 unmodified lines
521527 if (access_mask)
522528 diff &= access_mask;
523529
524− /* Were we able to observe a value-change? */
525− if (diff != 0)
530+ /*
531+ * Check if we observed a value change.
532+ *
533+ * Also check if the data race should be ignored (the rules depend on
534+ * non-zero diff); if it is to be ignored, the below rules for
535+ * KCSAN_VALUE_CHANGE_MAYBE apply.
536+ */
537+ if (diff && !kcsan_ignore_data_race(size, type, old, new, diff))
526538 value_change = KCSAN_VALUE_CHANGE_TRUE;
527539
528540 /* Check if this access raced with another. */
@@ -644,6 +656,15 @@ void __init kcsan_init(void) · 115 unmodified lines
644656 pr_info("enabled early\n");
645657 WRITE_ONCE(kcsan_enabled, true);
646658 }
659+
660+ if (IS_ENABLED(CONFIG_KCSAN_REPORT_VALUE_CHANGE_ONLY) ||
661+ IS_ENABLED(CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC) ||
662+ IS_ENABLED(CONFIG_KCSAN_PERMISSIVE) ||
663+ IS_ENABLED(CONFIG_KCSAN_IGNORE_ATOMICS)) {
664+ pr_warn("non-strict mode configured - use CONFIG_KCSAN_STRICT=y to see all data races\n");
665+ } else {
666+ pr_info("strict mode configured\n");
667+ }
647668 }
648669
649670 /* === Exported interface =================================================== */
kernel/kcsan/kcsan_test.c+32−0
@@ -414,6 +414,14 @@ static noinline void test_kernel_atomic_builtins(void) · 413 unmodified lines
414414 __atomic_load_n(&test_var, __ATOMIC_RELAXED);
415415 }
416416
417+static noinline void test_kernel_xor_1bit(void)
418+{
419+ /* Do not report data races between the read-writes. */
420+ kcsan_nestable_atomic_begin();
421+ test_var ^= 0x10000;
422+ kcsan_nestable_atomic_end();
423+}
424+
417425 /* ===== Test cases ===== */
418426
419427 /* Simple test with normal data race. */
@@ -952,6 +960,29 @@ static void test_atomic_builtins(struct kunit *test) · 532 unmodified lines
952960 KUNIT_EXPECT_FALSE(test, match_never);
953961 }
954962
963+__no_kcsan
964+static void test_1bit_value_change(struct kunit *test)
965+{
966+ const struct expect_report expect = {
967+ .access = {
968+ { test_kernel_read, &test_var, sizeof(test_var), 0 },
969+ { test_kernel_xor_1bit, &test_var, sizeof(test_var), __KCSAN_ACCESS_RW(KCSAN_ACCESS_WRITE) },
970+ },
971+ };
972+ bool match = false;
973+
974+ begin_test_checks(test_kernel_read, test_kernel_xor_1bit);
975+ do {
976+ match = IS_ENABLED(CONFIG_KCSAN_PERMISSIVE)
977+ ? report_available()
978+ : report_matches(&expect);
979+ } while (!end_test_checks(match));
980+ if (IS_ENABLED(CONFIG_KCSAN_PERMISSIVE))
981+ KUNIT_EXPECT_FALSE(test, match);
982+ else
983+ KUNIT_EXPECT_TRUE(test, match);
984+}
985+
955986 /*
956987 * Generate thread counts for all test cases. Values generated are in interval
957988 * [2, 5] followed by exponentially increasing thread counts from 8 to 32.
@@ -1024,6 +1055,7 @@ static struct kunit_case kcsan_test_cases[] = { · 66 unmodified lines
10241055 KCSAN_KUNIT_CASE(test_jiffies_noreport),
10251056 KCSAN_KUNIT_CASE(test_seqlock_noreport),
10261057 KCSAN_KUNIT_CASE(test_atomic_builtins),
1058+ KCSAN_KUNIT_CASE(test_1bit_value_change),
10271059 {},
10281060 };
10291061
kernel/kcsan/permissive.h+94−0
@@ -0,0 +1,94 @@
1+/* SPDX-License-Identifier: GPL-2.0 */
2+/*
3+ * Special rules for ignoring entire classes of data-racy memory accesses. None
4+ * of the rules here imply that such data races are generally safe!
5+ *
6+ * All rules in this file can be configured via CONFIG_KCSAN_PERMISSIVE. Keep
7+ * them separate from core code to make it easier to audit.
8+ *
9+ * Copyright (C) 2019, Google LLC.
10+ */
11+
12+#ifndef _KERNEL_KCSAN_PERMISSIVE_H
13+#define _KERNEL_KCSAN_PERMISSIVE_H
14+
15+#include <linux/bitops.h>
16+#include <linux/sched.h>
17+#include <linux/types.h>
18+
19+/*
20+ * Access ignore rules based on address.
21+ */
22+static __always_inline bool kcsan_ignore_address(const volatile void *ptr)
23+{
24+ if (!IS_ENABLED(CONFIG_KCSAN_PERMISSIVE))
25+ return false;
26+
27+ /*
28+ * Data-racy bitops on current->flags are too common, ignore completely
29+ * for now.
30+ */
31+ return ptr == &current->flags;
32+}
33+
34+/*
35+ * Data race ignore rules based on access type and value change patterns.
36+ */
37+static bool
38+kcsan_ignore_data_race(size_t size, int type, u64 old, u64 new, u64 diff)
39+{
40+ if (!IS_ENABLED(CONFIG_KCSAN_PERMISSIVE))
41+ return false;
42+
43+ /*
44+ * Rules here are only for plain read accesses, so that we still report
45+ * data races between plain read-write accesses.
46+ */
47+ if (type || size > sizeof(long))
48+ return false;
49+
50+ /*
51+ * A common pattern is checking/setting just 1 bit in a variable; for
52+ * example:
53+ *
54+ * if (flags & SOME_FLAG) { ... }
55+ *
56+ * and elsewhere flags is updated concurrently:
57+ *
58+ * flags |= SOME_OTHER_FLAG; // just 1 bit
59+ *
60+ * While it is still recommended that such accesses be marked
61+ * appropriately, in many cases these types of data races are so common
62+ * that marking them all is often unrealistic and left to maintainer
63+ * preference.
64+ *
65+ * The assumption in all cases is that with all known compiler
66+ * optimizations (including those that tear accesses), because no more
67+ * than 1 bit changed, the plain accesses are safe despite the presence
68+ * of data races.
69+ *
70+ * The rules here will ignore the data races if we observe no more than
71+ * 1 bit changed.
72+ *
73+ * Of course many operations can effecively change just 1 bit, but the
74+ * general assuption that data races involving 1-bit changes can be
75+ * tolerated still applies.
76+ *
77+ * And in case a true bug is missed, the bug likely manifests as a
78+ * reportable data race elsewhere.
79+ */
80+ if (hweight64(diff) == 1) {
81+ /*
82+ * Exception: Report data races where the values look like
83+ * ordinary booleans (one of them was 0 and the 0th bit was
84+ * changed) More often than not, they come with interesting
85+ * memory ordering requirements, so let's report them.
86+ */
87+ if (!((!old || !new) && diff == 1))
88+ return true;
89+ }
90+
91+ return false;
92+}
93+
94+#endif /* _KERNEL_KCSAN_PERMISSIVE_H */

Keyboard shortcuts

General

Show keyboard shortcuts
?
Focus the current search
/

Issues and pull requests

Open filters
f

Issues, pull requests and Inbox

Select next item
j↓
Select previous item
k↑
Open selected item
Enter
Dismiss filters or clear selection
Esc

Repository code

Go to file
t