From 6f2d98192c3f204592434177ba240564346eed9f Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Wed, 14 Apr 2021 13:28:17 +0200 Subject: kcsan: Simplify value change detection In kcsan_setup_watchpoint() we store snapshots of a watched value into a union of u8/u16/u32/u64 sized fields, modify this in place using a consistent field, then later check for any changes via the u64 field. We can achieve the safe effect more simply by always treating the field as a u64, as smaller values will be zero-extended. As the values are zero-extended, we don't need to truncate the access_mask when we apply it, and can always apply the full 64-bit access_mask to the 64-bit value. Finally, we can store the two snapshots and calculated difference separately, which makes the code a little easier to read, and will permit reporting the old/new values in subsequent patches. There should be no functional change as a result of this patch. Signed-off-by: Mark Rutland Signed-off-by: Marco Elver Signed-off-by: Paul E. McKenney --- kernel/kcsan/core.c | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-) (limited to 'kernel') diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c index 45c821d4e8bd..d360183002d6 100644 --- a/kernel/kcsan/core.c +++ b/kernel/kcsan/core.c @@ -407,12 +407,7 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type) const bool is_write = (type & KCSAN_ACCESS_WRITE) != 0; const bool is_assert = (type & KCSAN_ACCESS_ASSERT) != 0; atomic_long_t *watchpoint; - union { - u8 _1; - u16 _2; - u32 _4; - u64 _8; - } expect_value; + u64 old, new, diff; unsigned long access_mask; enum kcsan_value_change value_change = KCSAN_VALUE_CHANGE_MAYBE; unsigned long ua_flags = user_access_save(); @@ -468,19 +463,19 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type) * Read the current value, to later check and infer a race if the data * was modified via a non-instrumented access, e.g. from a device. */ - expect_value._8 = 0; + old = 0; switch (size) { case 1: - expect_value._1 = READ_ONCE(*(const u8 *)ptr); + old = READ_ONCE(*(const u8 *)ptr); break; case 2: - expect_value._2 = READ_ONCE(*(const u16 *)ptr); + old = READ_ONCE(*(const u16 *)ptr); break; case 4: - expect_value._4 = READ_ONCE(*(const u32 *)ptr); + old = READ_ONCE(*(const u32 *)ptr); break; case 8: - expect_value._8 = READ_ONCE(*(const u64 *)ptr); + old = READ_ONCE(*(const u64 *)ptr); break; default: break; /* ignore; we do not diff the values */ @@ -506,33 +501,30 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type) * racy access. */ access_mask = get_ctx()->access_mask; + new = 0; switch (size) { case 1: - expect_value._1 ^= READ_ONCE(*(const u8 *)ptr); - if (access_mask) - expect_value._1 &= (u8)access_mask; + new = READ_ONCE(*(const u8 *)ptr); break; case 2: - expect_value._2 ^= READ_ONCE(*(const u16 *)ptr); - if (access_mask) - expect_value._2 &= (u16)access_mask; + new = READ_ONCE(*(const u16 *)ptr); break; case 4: - expect_value._4 ^= READ_ONCE(*(const u32 *)ptr); - if (access_mask) - expect_value._4 &= (u32)access_mask; + new = READ_ONCE(*(const u32 *)ptr); break; case 8: - expect_value._8 ^= READ_ONCE(*(const u64 *)ptr); - if (access_mask) - expect_value._8 &= (u64)access_mask; + new = READ_ONCE(*(const u64 *)ptr); break; default: break; /* ignore; we do not diff the values */ } + diff = old ^ new; + if (access_mask) + diff &= access_mask; + /* Were we able to observe a value-change? */ - if (expect_value._8 != 0) + if (diff != 0) value_change = KCSAN_VALUE_CHANGE_TRUE; /* Check if this access raced with another. */ -- cgit v1.2.3 From 793c2579beefa95894fc0afbbdc1a80a4e3bf306 Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Wed, 14 Apr 2021 13:28:18 +0200 Subject: kcsan: Distinguish kcsan_report() calls Currently kcsan_report() is used to handle three distinct cases: * The caller hit a watchpoint when attempting an access. Some information regarding the caller and access are recorded, but no output is produced. * A caller which previously setup a watchpoint detected that the watchpoint has been hit, and possibly detected a change to the location in memory being watched. This may result in output reporting the interaction between this caller and the caller which hit the watchpoint. * A caller detected a change to a modification to a memory location which wasn't detected by a watchpoint, for which there is no information on the other thread. This may result in output reporting the unexpected change. ... depending on the specific case the caller has distinct pieces of information available, but the prototype of kcsan_report() has to handle all three cases. This means that in some cases we pass redundant information, and in others we don't pass all the information we could pass. This also means that the report code has to demux these three cases. So that we can pass some additional information while also simplifying the callers and report code, add separate kcsan_report_*() functions for the distinct cases, updating callers accordingly. As the watchpoint_idx is unused in the case of kcsan_report_unknown_origin(), this passes a dummy value into kcsan_report(). Subsequent patches will refactor the report code to avoid this. There should be no functional change as a result of this patch. Signed-off-by: Mark Rutland [ elver@google.com: try to make kcsan_report_*() names more descriptive ] Signed-off-by: Marco Elver Signed-off-by: Paul E. McKenney --- kernel/kcsan/core.c | 12 ++++-------- kernel/kcsan/kcsan.h | 10 ++++++---- kernel/kcsan/report.c | 26 +++++++++++++++++++++++--- 3 files changed, 33 insertions(+), 15 deletions(-) (limited to 'kernel') diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c index d360183002d6..6fe1513e1e6a 100644 --- a/kernel/kcsan/core.c +++ b/kernel/kcsan/core.c @@ -380,9 +380,7 @@ static noinline void kcsan_found_watchpoint(const volatile void *ptr, if (consumed) { kcsan_save_irqtrace(current); - kcsan_report(ptr, size, type, KCSAN_VALUE_CHANGE_MAYBE, - KCSAN_REPORT_CONSUMED_WATCHPOINT, - watchpoint - watchpoints); + kcsan_report_set_info(ptr, size, type, watchpoint - watchpoints); kcsan_restore_irqtrace(current); } else { /* @@ -558,8 +556,8 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type) if (is_assert && value_change == KCSAN_VALUE_CHANGE_TRUE) atomic_long_inc(&kcsan_counters[KCSAN_COUNTER_ASSERT_FAILURES]); - kcsan_report(ptr, size, type, value_change, KCSAN_REPORT_RACE_SIGNAL, - watchpoint - watchpoints); + kcsan_report_known_origin(ptr, size, type, value_change, + watchpoint - watchpoints); } else if (value_change == KCSAN_VALUE_CHANGE_TRUE) { /* Inferring a race, since the value should not have changed. */ @@ -568,9 +566,7 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type) atomic_long_inc(&kcsan_counters[KCSAN_COUNTER_ASSERT_FAILURES]); if (IS_ENABLED(CONFIG_KCSAN_REPORT_RACE_UNKNOWN_ORIGIN) || is_assert) - kcsan_report(ptr, size, type, KCSAN_VALUE_CHANGE_TRUE, - KCSAN_REPORT_RACE_UNKNOWN_ORIGIN, - watchpoint - watchpoints); + kcsan_report_unknown_origin(ptr, size, type); } /* diff --git a/kernel/kcsan/kcsan.h b/kernel/kcsan/kcsan.h index 9881099d4179..2ee43fd5d6a4 100644 --- a/kernel/kcsan/kcsan.h +++ b/kernel/kcsan/kcsan.h @@ -136,10 +136,12 @@ enum kcsan_report_type { }; /* - * Print a race report from thread that encountered the race. + * Notify the report code that a race occurred. */ -extern void kcsan_report(const volatile void *ptr, size_t size, int access_type, - enum kcsan_value_change value_change, - enum kcsan_report_type type, int watchpoint_idx); +void kcsan_report_set_info(const volatile void *ptr, size_t size, int access_type, + int watchpoint_idx); +void kcsan_report_known_origin(const volatile void *ptr, size_t size, int access_type, + enum kcsan_value_change value_change, int watchpoint_idx); +void kcsan_report_unknown_origin(const volatile void *ptr, size_t size, int access_type); #endif /* _KERNEL_KCSAN_KCSAN_H */ diff --git a/kernel/kcsan/report.c b/kernel/kcsan/report.c index 13dce3c664d6..5232bf218ea7 100644 --- a/kernel/kcsan/report.c +++ b/kernel/kcsan/report.c @@ -598,9 +598,9 @@ static noinline bool prepare_report(unsigned long *flags, } } -void kcsan_report(const volatile void *ptr, size_t size, int access_type, - enum kcsan_value_change value_change, - enum kcsan_report_type type, int watchpoint_idx) +static void kcsan_report(const volatile void *ptr, size_t size, int access_type, + enum kcsan_value_change value_change, + enum kcsan_report_type type, int watchpoint_idx) { unsigned long flags = 0; const struct access_info ai = { @@ -645,3 +645,23 @@ void kcsan_report(const volatile void *ptr, size_t size, int access_type, out: kcsan_enable_current(); } + +void kcsan_report_set_info(const volatile void *ptr, size_t size, int access_type, + int watchpoint_idx) +{ + kcsan_report(ptr, size, access_type, KCSAN_VALUE_CHANGE_MAYBE, + KCSAN_REPORT_CONSUMED_WATCHPOINT, watchpoint_idx); +} + +void kcsan_report_known_origin(const volatile void *ptr, size_t size, int access_type, + enum kcsan_value_change value_change, int watchpoint_idx) +{ + kcsan_report(ptr, size, access_type, value_change, + KCSAN_REPORT_RACE_SIGNAL, watchpoint_idx); +} + +void kcsan_report_unknown_origin(const volatile void *ptr, size_t size, int access_type) +{ + kcsan_report(ptr, size, access_type, KCSAN_VALUE_CHANGE_TRUE, + KCSAN_REPORT_RACE_UNKNOWN_ORIGIN, 0); +} -- cgit v1.2.3 From 95f7524d7f0c6fddbc24fb623d61b7d508626f41 Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Wed, 14 Apr 2021 13:28:19 +0200 Subject: kcsan: Refactor passing watchpoint/other_info The `watchpoint_idx` argument to kcsan_report() isn't meaningful for races which were not detected by a watchpoint, and it would be clearer if callers passed the other_info directly so that a NULL value can be passed in this case. Given that callers manipulate their watchpoints before passing the index into kcsan_report_*(), and given we index the `other_infos` array using this before we sanity-check it, the subsequent sanity check isn't all that useful. Let's remove the `watchpoint_idx` sanity check, and move the job of finding the `other_info` out of kcsan_report(). Other than the removal of the check, there should be no functional change as a result of this patch. Signed-off-by: Mark Rutland Signed-off-by: Marco Elver Signed-off-by: Paul E. McKenney --- kernel/kcsan/report.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) (limited to 'kernel') diff --git a/kernel/kcsan/report.c b/kernel/kcsan/report.c index 5232bf218ea7..88225f6d471e 100644 --- a/kernel/kcsan/report.c +++ b/kernel/kcsan/report.c @@ -600,7 +600,7 @@ static noinline bool prepare_report(unsigned long *flags, static void kcsan_report(const volatile void *ptr, size_t size, int access_type, enum kcsan_value_change value_change, - enum kcsan_report_type type, int watchpoint_idx) + enum kcsan_report_type type, struct other_info *other_info) { unsigned long flags = 0; const struct access_info ai = { @@ -610,12 +610,8 @@ static void kcsan_report(const volatile void *ptr, size_t size, int access_type, .task_pid = in_task() ? task_pid_nr(current) : -1, .cpu_id = raw_smp_processor_id() }; - struct other_info *other_info = type == KCSAN_REPORT_RACE_UNKNOWN_ORIGIN - ? NULL : &other_infos[watchpoint_idx]; kcsan_disable_current(); - if (WARN_ON(watchpoint_idx < 0 || watchpoint_idx >= ARRAY_SIZE(other_infos))) - goto out; /* * Because we may generate reports when we're in scheduler code, the use @@ -642,7 +638,6 @@ static void kcsan_report(const volatile void *ptr, size_t size, int access_type, } lockdep_on(); -out: kcsan_enable_current(); } @@ -650,18 +645,18 @@ void kcsan_report_set_info(const volatile void *ptr, size_t size, int access_typ int watchpoint_idx) { kcsan_report(ptr, size, access_type, KCSAN_VALUE_CHANGE_MAYBE, - KCSAN_REPORT_CONSUMED_WATCHPOINT, watchpoint_idx); + KCSAN_REPORT_CONSUMED_WATCHPOINT, &other_infos[watchpoint_idx]); } void kcsan_report_known_origin(const volatile void *ptr, size_t size, int access_type, enum kcsan_value_change value_change, int watchpoint_idx) { kcsan_report(ptr, size, access_type, value_change, - KCSAN_REPORT_RACE_SIGNAL, watchpoint_idx); + KCSAN_REPORT_RACE_SIGNAL, &other_infos[watchpoint_idx]); } void kcsan_report_unknown_origin(const volatile void *ptr, size_t size, int access_type) { kcsan_report(ptr, size, access_type, KCSAN_VALUE_CHANGE_TRUE, - KCSAN_REPORT_RACE_UNKNOWN_ORIGIN, 0); + KCSAN_REPORT_RACE_UNKNOWN_ORIGIN, NULL); } -- cgit v1.2.3 From 97aa6139e1b506795ab19941b1c3851042199788 Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Wed, 14 Apr 2021 13:28:20 +0200 Subject: kcsan: Fold panic() call into print_report() So that we can add more callers of print_report(), lets fold the panic() call into print_report() so the caller doesn't have to handle this explicitly. There should be no functional change as a result of this patch. Signed-off-by: Mark Rutland Signed-off-by: Marco Elver Signed-off-by: Paul E. McKenney --- kernel/kcsan/report.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) (limited to 'kernel') diff --git a/kernel/kcsan/report.c b/kernel/kcsan/report.c index 88225f6d471e..8bfa970965a1 100644 --- a/kernel/kcsan/report.c +++ b/kernel/kcsan/report.c @@ -325,10 +325,7 @@ static void print_verbose_info(struct task_struct *task) print_irqtrace_events(task); } -/* - * Returns true if a report was generated, false otherwise. - */ -static bool print_report(enum kcsan_value_change value_change, +static void print_report(enum kcsan_value_change value_change, enum kcsan_report_type type, const struct access_info *ai, const struct other_info *other_info) @@ -344,7 +341,7 @@ static bool print_report(enum kcsan_value_change value_change, * Must check report filter rules before starting to print. */ if (skip_report(KCSAN_VALUE_CHANGE_TRUE, stack_entries[skipnr])) - return false; + return; if (type == KCSAN_REPORT_RACE_SIGNAL) { other_skipnr = get_stack_skipnr(other_info->stack_entries, @@ -353,11 +350,11 @@ static bool print_report(enum kcsan_value_change value_change, /* @value_change is only known for the other thread */ if (skip_report(value_change, other_frame)) - return false; + return; } if (rate_limit_report(this_frame, other_frame)) - return false; + return; /* Print report header. */ pr_err("==================================================================\n"); @@ -431,7 +428,8 @@ static bool print_report(enum kcsan_value_change value_change, dump_stack_print_info(KERN_DEFAULT); pr_err("==================================================================\n"); - return true; + if (panic_on_warn) + panic("panic_on_warn set ...\n"); } static void release_report(unsigned long *flags, struct other_info *other_info) @@ -628,11 +626,8 @@ static void kcsan_report(const volatile void *ptr, size_t size, int access_type, * either TRUE or MAYBE. In case of MAYBE, further filtering may * be done once we know the full stack trace in print_report(). */ - bool reported = value_change != KCSAN_VALUE_CHANGE_FALSE && - print_report(value_change, type, &ai, other_info); - - if (reported && panic_on_warn) - panic("panic_on_warn set ...\n"); + if (value_change != KCSAN_VALUE_CHANGE_FALSE) + print_report(value_change, type, &ai, other_info); release_report(&flags, other_info); } -- cgit v1.2.3 From 39b2e763f2defe326e960daefb7fe6acbb2a95b1 Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Wed, 14 Apr 2021 13:28:21 +0200 Subject: kcsan: Refactor access_info initialization In subsequent patches we'll want to split kcsan_report() into distinct handlers for each report type. The largest bit of common work is initializing the `access_info`, so let's factor this out into a helper, and have the kcsan_report_*() functions pass the `aaccess_info` as a parameter to kcsan_report(). There should be no functional change as a result of this patch. Signed-off-by: Mark Rutland Signed-off-by: Marco Elver Signed-off-by: Paul E. McKenney --- kernel/kcsan/report.c | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) (limited to 'kernel') diff --git a/kernel/kcsan/report.c b/kernel/kcsan/report.c index 8bfa970965a1..d8441bed065c 100644 --- a/kernel/kcsan/report.c +++ b/kernel/kcsan/report.c @@ -596,18 +596,10 @@ static noinline bool prepare_report(unsigned long *flags, } } -static void kcsan_report(const volatile void *ptr, size_t size, int access_type, - enum kcsan_value_change value_change, +static void kcsan_report(const struct access_info *ai, enum kcsan_value_change value_change, enum kcsan_report_type type, struct other_info *other_info) { unsigned long flags = 0; - const struct access_info ai = { - .ptr = ptr, - .size = size, - .access_type = access_type, - .task_pid = in_task() ? task_pid_nr(current) : -1, - .cpu_id = raw_smp_processor_id() - }; kcsan_disable_current(); @@ -620,14 +612,14 @@ static void kcsan_report(const volatile void *ptr, size_t size, int access_type, */ lockdep_off(); - if (prepare_report(&flags, type, &ai, other_info)) { + if (prepare_report(&flags, type, ai, other_info)) { /* * Never report if value_change is FALSE, only if we it is * either TRUE or MAYBE. In case of MAYBE, further filtering may * be done once we know the full stack trace in print_report(). */ if (value_change != KCSAN_VALUE_CHANGE_FALSE) - print_report(value_change, type, &ai, other_info); + print_report(value_change, type, ai, other_info); release_report(&flags, other_info); } @@ -636,22 +628,38 @@ static void kcsan_report(const volatile void *ptr, size_t size, int access_type, kcsan_enable_current(); } +static struct access_info prepare_access_info(const volatile void *ptr, size_t size, + int access_type) +{ + return (struct access_info) { + .ptr = ptr, + .size = size, + .access_type = access_type, + .task_pid = in_task() ? task_pid_nr(current) : -1, + .cpu_id = raw_smp_processor_id() + }; +} + void kcsan_report_set_info(const volatile void *ptr, size_t size, int access_type, int watchpoint_idx) { - kcsan_report(ptr, size, access_type, KCSAN_VALUE_CHANGE_MAYBE, - KCSAN_REPORT_CONSUMED_WATCHPOINT, &other_infos[watchpoint_idx]); + const struct access_info ai = prepare_access_info(ptr, size, access_type); + + kcsan_report(&ai, KCSAN_VALUE_CHANGE_MAYBE, KCSAN_REPORT_CONSUMED_WATCHPOINT, + &other_infos[watchpoint_idx]); } void kcsan_report_known_origin(const volatile void *ptr, size_t size, int access_type, enum kcsan_value_change value_change, int watchpoint_idx) { - kcsan_report(ptr, size, access_type, value_change, - KCSAN_REPORT_RACE_SIGNAL, &other_infos[watchpoint_idx]); + const struct access_info ai = prepare_access_info(ptr, size, access_type); + + kcsan_report(&ai, value_change, KCSAN_REPORT_RACE_SIGNAL, &other_infos[watchpoint_idx]); } void kcsan_report_unknown_origin(const volatile void *ptr, size_t size, int access_type) { - kcsan_report(ptr, size, access_type, KCSAN_VALUE_CHANGE_TRUE, - KCSAN_REPORT_RACE_UNKNOWN_ORIGIN, NULL); + const struct access_info ai = prepare_access_info(ptr, size, access_type); + + kcsan_report(&ai, KCSAN_VALUE_CHANGE_TRUE, KCSAN_REPORT_RACE_UNKNOWN_ORIGIN, NULL); } -- cgit v1.2.3 From 19dfdc05ffed960024e175db21c8e11ef96daeee Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Wed, 14 Apr 2021 13:28:22 +0200 Subject: kcsan: Remove reporting indirection Now that we have separate kcsan_report_*() functions, we can factor the distinct logic for each of the report cases out of kcsan_report(). While this means each case has to handle mutual exclusion independently, this minimizes the conditionality of code and makes it easier to read, and will permit passing distinct bits of information to print_report() in future. There should be no functional change as a result of this patch. Signed-off-by: Mark Rutland [ elver@google.com: retain comment about lockdep_off() ] Signed-off-by: Marco Elver Signed-off-by: Paul E. McKenney --- kernel/kcsan/report.c | 115 +++++++++++++++++++++----------------------------- 1 file changed, 49 insertions(+), 66 deletions(-) (limited to 'kernel') diff --git a/kernel/kcsan/report.c b/kernel/kcsan/report.c index d8441bed065c..ba924f110c95 100644 --- a/kernel/kcsan/report.c +++ b/kernel/kcsan/report.c @@ -434,13 +434,11 @@ static void print_report(enum kcsan_value_change value_change, static void release_report(unsigned long *flags, struct other_info *other_info) { - if (other_info) - /* - * Use size to denote valid/invalid, since KCSAN entirely - * ignores 0-sized accesses. - */ - other_info->ai.size = 0; - + /* + * Use size to denote valid/invalid, since KCSAN entirely ignores + * 0-sized accesses. + */ + other_info->ai.size = 0; raw_spin_unlock_irqrestore(&report_lock, *flags); } @@ -573,61 +571,6 @@ discard: return false; } -/* - * Depending on the report type either sets @other_info and returns false, or - * awaits @other_info and returns true. If @other_info is not required for the - * report type, simply acquires @report_lock and returns true. - */ -static noinline bool prepare_report(unsigned long *flags, - enum kcsan_report_type type, - const struct access_info *ai, - struct other_info *other_info) -{ - switch (type) { - case KCSAN_REPORT_CONSUMED_WATCHPOINT: - prepare_report_producer(flags, ai, other_info); - return false; - case KCSAN_REPORT_RACE_SIGNAL: - return prepare_report_consumer(flags, ai, other_info); - default: - /* @other_info not required; just acquire @report_lock. */ - raw_spin_lock_irqsave(&report_lock, *flags); - return true; - } -} - -static void kcsan_report(const struct access_info *ai, enum kcsan_value_change value_change, - enum kcsan_report_type type, struct other_info *other_info) -{ - unsigned long flags = 0; - - kcsan_disable_current(); - - /* - * Because we may generate reports when we're in scheduler code, the use - * of printk() could deadlock. Until such time that all printing code - * called in print_report() is scheduler-safe, accept the risk, and just - * get our message out. As such, also disable lockdep to hide the - * warning, and avoid disabling lockdep for the rest of the kernel. - */ - lockdep_off(); - - if (prepare_report(&flags, type, ai, other_info)) { - /* - * Never report if value_change is FALSE, only if we it is - * either TRUE or MAYBE. In case of MAYBE, further filtering may - * be done once we know the full stack trace in print_report(). - */ - if (value_change != KCSAN_VALUE_CHANGE_FALSE) - print_report(value_change, type, ai, other_info); - - release_report(&flags, other_info); - } - - lockdep_on(); - kcsan_enable_current(); -} - static struct access_info prepare_access_info(const volatile void *ptr, size_t size, int access_type) { @@ -644,22 +587,62 @@ void kcsan_report_set_info(const volatile void *ptr, size_t size, int access_typ int watchpoint_idx) { const struct access_info ai = prepare_access_info(ptr, size, access_type); + unsigned long flags; + + kcsan_disable_current(); + lockdep_off(); /* See kcsan_report_known_origin(). */ - kcsan_report(&ai, KCSAN_VALUE_CHANGE_MAYBE, KCSAN_REPORT_CONSUMED_WATCHPOINT, - &other_infos[watchpoint_idx]); + prepare_report_producer(&flags, &ai, &other_infos[watchpoint_idx]); + + lockdep_on(); + kcsan_enable_current(); } void kcsan_report_known_origin(const volatile void *ptr, size_t size, int access_type, enum kcsan_value_change value_change, int watchpoint_idx) { const struct access_info ai = prepare_access_info(ptr, size, access_type); + struct other_info *other_info = &other_infos[watchpoint_idx]; + unsigned long flags = 0; - kcsan_report(&ai, value_change, KCSAN_REPORT_RACE_SIGNAL, &other_infos[watchpoint_idx]); + kcsan_disable_current(); + /* + * Because we may generate reports when we're in scheduler code, the use + * of printk() could deadlock. Until such time that all printing code + * called in print_report() is scheduler-safe, accept the risk, and just + * get our message out. As such, also disable lockdep to hide the + * warning, and avoid disabling lockdep for the rest of the kernel. + */ + lockdep_off(); + + if (!prepare_report_consumer(&flags, &ai, other_info)) + goto out; + /* + * Never report if value_change is FALSE, only when it is + * either TRUE or MAYBE. In case of MAYBE, further filtering may + * be done once we know the full stack trace in print_report(). + */ + if (value_change != KCSAN_VALUE_CHANGE_FALSE) + print_report(value_change, KCSAN_REPORT_RACE_SIGNAL, &ai, other_info); + + release_report(&flags, other_info); +out: + lockdep_on(); + kcsan_enable_current(); } void kcsan_report_unknown_origin(const volatile void *ptr, size_t size, int access_type) { const struct access_info ai = prepare_access_info(ptr, size, access_type); + unsigned long flags; + + kcsan_disable_current(); + lockdep_off(); /* See kcsan_report_known_origin(). */ + + raw_spin_lock_irqsave(&report_lock, flags); + print_report(KCSAN_VALUE_CHANGE_TRUE, KCSAN_REPORT_RACE_UNKNOWN_ORIGIN, &ai, NULL); + raw_spin_unlock_irqrestore(&report_lock, flags); - kcsan_report(&ai, KCSAN_VALUE_CHANGE_TRUE, KCSAN_REPORT_RACE_UNKNOWN_ORIGIN, NULL); + lockdep_on(); + kcsan_enable_current(); } -- cgit v1.2.3 From 609f809746458522a7a96132acf0ca7ee67c424c Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Wed, 14 Apr 2021 13:28:23 +0200 Subject: kcsan: Remove kcsan_report_type Now that the reporting code has been refactored, it's clear by construction that print_report() can only be passed KCSAN_REPORT_RACE_SIGNAL or KCSAN_REPORT_RACE_UNKNOWN_ORIGIN, and these can also be distinguished by the presence of `other_info`. Let's simplify things and remove the report type enum, and instead let's check `other_info` to distinguish these cases. This allows us to remove code for cases which are impossible and generally makes the code simpler. There should be no functional change as a result of this patch. Signed-off-by: Mark Rutland [ elver@google.com: add updated comments to kcsan_report_*() functions ] Signed-off-by: Marco Elver Signed-off-by: Paul E. McKenney --- kernel/kcsan/kcsan.h | 33 +++++++++++++-------------------- kernel/kcsan/report.c | 29 +++++++---------------------- 2 files changed, 20 insertions(+), 42 deletions(-) (limited to 'kernel') diff --git a/kernel/kcsan/kcsan.h b/kernel/kcsan/kcsan.h index 2ee43fd5d6a4..572f119a19eb 100644 --- a/kernel/kcsan/kcsan.h +++ b/kernel/kcsan/kcsan.h @@ -116,32 +116,25 @@ enum kcsan_value_change { KCSAN_VALUE_CHANGE_TRUE, }; -enum kcsan_report_type { - /* - * The thread that set up the watchpoint and briefly stalled was - * signalled that another thread triggered the watchpoint. - */ - KCSAN_REPORT_RACE_SIGNAL, - - /* - * A thread found and consumed a matching watchpoint. - */ - KCSAN_REPORT_CONSUMED_WATCHPOINT, - - /* - * No other thread was observed to race with the access, but the data - * value before and after the stall differs. - */ - KCSAN_REPORT_RACE_UNKNOWN_ORIGIN, -}; - /* - * Notify the report code that a race occurred. + * The calling thread hit and consumed a watchpoint: set the access information + * to be consumed by the reporting thread. No report is printed yet. */ void kcsan_report_set_info(const volatile void *ptr, size_t size, int access_type, int watchpoint_idx); + +/* + * The calling thread observed that the watchpoint it set up was hit and + * consumed: print the full report based on information set by the racing + * thread. + */ void kcsan_report_known_origin(const volatile void *ptr, size_t size, int access_type, enum kcsan_value_change value_change, int watchpoint_idx); + +/* + * No other thread was observed to race with the access, but the data value + * before and after the stall differs. Reports a race of "unknown origin". + */ void kcsan_report_unknown_origin(const volatile void *ptr, size_t size, int access_type); #endif /* _KERNEL_KCSAN_KCSAN_H */ diff --git a/kernel/kcsan/report.c b/kernel/kcsan/report.c index ba924f110c95..50cee2357885 100644 --- a/kernel/kcsan/report.c +++ b/kernel/kcsan/report.c @@ -326,7 +326,6 @@ static void print_verbose_info(struct task_struct *task) } static void print_report(enum kcsan_value_change value_change, - enum kcsan_report_type type, const struct access_info *ai, const struct other_info *other_info) { @@ -343,7 +342,7 @@ static void print_report(enum kcsan_value_change value_change, if (skip_report(KCSAN_VALUE_CHANGE_TRUE, stack_entries[skipnr])) return; - if (type == KCSAN_REPORT_RACE_SIGNAL) { + if (other_info) { other_skipnr = get_stack_skipnr(other_info->stack_entries, other_info->num_stack_entries); other_frame = other_info->stack_entries[other_skipnr]; @@ -358,8 +357,7 @@ static void print_report(enum kcsan_value_change value_change, /* Print report header. */ pr_err("==================================================================\n"); - switch (type) { - case KCSAN_REPORT_RACE_SIGNAL: { + if (other_info) { int cmp; /* @@ -371,22 +369,15 @@ static void print_report(enum kcsan_value_change value_change, get_bug_type(ai->access_type | other_info->ai.access_type), (void *)(cmp < 0 ? other_frame : this_frame), (void *)(cmp < 0 ? this_frame : other_frame)); - } break; - - case KCSAN_REPORT_RACE_UNKNOWN_ORIGIN: + } else { pr_err("BUG: KCSAN: %s in %pS\n", get_bug_type(ai->access_type), (void *)this_frame); - break; - - default: - BUG(); } pr_err("\n"); /* Print information about the racing accesses. */ - switch (type) { - case KCSAN_REPORT_RACE_SIGNAL: + if (other_info) { pr_err("%s to 0x%px of %zu bytes by %s on cpu %i:\n", get_access_type(other_info->ai.access_type), other_info->ai.ptr, other_info->ai.size, get_thread_desc(other_info->ai.task_pid), @@ -404,16 +395,10 @@ static void print_report(enum kcsan_value_change value_change, pr_err("%s to 0x%px of %zu bytes by %s on cpu %i:\n", get_access_type(ai->access_type), ai->ptr, ai->size, get_thread_desc(ai->task_pid), ai->cpu_id); - break; - - case KCSAN_REPORT_RACE_UNKNOWN_ORIGIN: + } else { pr_err("race at unknown origin, with %s to 0x%px of %zu bytes by %s on cpu %i:\n", get_access_type(ai->access_type), ai->ptr, ai->size, get_thread_desc(ai->task_pid), ai->cpu_id); - break; - - default: - BUG(); } /* Print stack trace of this thread. */ stack_trace_print(stack_entries + skipnr, num_stack_entries - skipnr, @@ -623,7 +608,7 @@ void kcsan_report_known_origin(const volatile void *ptr, size_t size, int access * be done once we know the full stack trace in print_report(). */ if (value_change != KCSAN_VALUE_CHANGE_FALSE) - print_report(value_change, KCSAN_REPORT_RACE_SIGNAL, &ai, other_info); + print_report(value_change, &ai, other_info); release_report(&flags, other_info); out: @@ -640,7 +625,7 @@ void kcsan_report_unknown_origin(const volatile void *ptr, size_t size, int acce lockdep_off(); /* See kcsan_report_known_origin(). */ raw_spin_lock_irqsave(&report_lock, flags); - print_report(KCSAN_VALUE_CHANGE_TRUE, KCSAN_REPORT_RACE_UNKNOWN_ORIGIN, &ai, NULL); + print_report(KCSAN_VALUE_CHANGE_TRUE, &ai, NULL); raw_spin_unlock_irqrestore(&report_lock, flags); lockdep_on(); -- cgit v1.2.3 From 7bbe6dc0ade7e394ee1568dc9979fd0e3e155435 Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Wed, 14 Apr 2021 13:28:24 +0200 Subject: kcsan: Report observed value changes When a thread detects that a memory location was modified without its watchpoint being hit, the report notes that a change was detected, but does not provide concrete values for the change. Knowing the concrete values can be very helpful in tracking down any racy writers (e.g. as specific values may only be written in some portions of code, or under certain conditions). When we detect a modification, let's report the concrete old/new values, along with the access's mask of relevant bits (and which relevant bits were modified). This can make it easier to identify potential racy writers. As the snapshots are at most 8 bytes, we can only report values for acceses up to this size, but this appears to cater for the common case. When we detect a race via a watchpoint, we may or may not have concrete values for the modification. To be helpful, let's attempt to log them when we do as they can be ignored where irrelevant. The resulting reports appears as follows, with values zero-padded to the access width: | ================================================================== | BUG: KCSAN: data-race in el0_svc_common+0x34/0x25c arch/arm64/kernel/syscall.c:96 | | race at unknown origin, with read to 0xffff00007ae6aa00 of 8 bytes by task 223 on cpu 1: | el0_svc_common+0x34/0x25c arch/arm64/kernel/syscall.c:96 | do_el0_svc+0x48/0xec arch/arm64/kernel/syscall.c:178 | el0_svc arch/arm64/kernel/entry-common.c:226 [inline] | el0_sync_handler+0x1a4/0x390 arch/arm64/kernel/entry-common.c:236 | el0_sync+0x140/0x180 arch/arm64/kernel/entry.S:674 | | value changed: 0x0000000000000000 -> 0x0000000000000002 | | Reported by Kernel Concurrency Sanitizer on: | CPU: 1 PID: 223 Comm: syz-executor.1 Not tainted 5.8.0-rc3-00094-ga73f923ecc8e-dirty #3 | Hardware name: linux,dummy-virt (DT) | ================================================================== If an access mask is set, it is shown underneath the "value changed" line as "bits changed: 0x with mask 0x". Signed-off-by: Mark Rutland [ elver@google.com: align "value changed" and "bits changed" lines, which required massaging the message; do not print bits+mask if no mask set. ] Signed-off-by: Marco Elver Signed-off-by: Paul E. McKenney --- kernel/kcsan/core.c | 5 +++-- kernel/kcsan/kcsan.h | 6 ++++-- kernel/kcsan/report.c | 31 ++++++++++++++++++++++++++----- 3 files changed, 33 insertions(+), 9 deletions(-) (limited to 'kernel') diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c index 6fe1513e1e6a..26709ea65c71 100644 --- a/kernel/kcsan/core.c +++ b/kernel/kcsan/core.c @@ -557,7 +557,8 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type) atomic_long_inc(&kcsan_counters[KCSAN_COUNTER_ASSERT_FAILURES]); kcsan_report_known_origin(ptr, size, type, value_change, - watchpoint - watchpoints); + watchpoint - watchpoints, + old, new, access_mask); } else if (value_change == KCSAN_VALUE_CHANGE_TRUE) { /* Inferring a race, since the value should not have changed. */ @@ -566,7 +567,7 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type) atomic_long_inc(&kcsan_counters[KCSAN_COUNTER_ASSERT_FAILURES]); if (IS_ENABLED(CONFIG_KCSAN_REPORT_RACE_UNKNOWN_ORIGIN) || is_assert) - kcsan_report_unknown_origin(ptr, size, type); + kcsan_report_unknown_origin(ptr, size, type, old, new, access_mask); } /* diff --git a/kernel/kcsan/kcsan.h b/kernel/kcsan/kcsan.h index 572f119a19eb..f36e25c497ed 100644 --- a/kernel/kcsan/kcsan.h +++ b/kernel/kcsan/kcsan.h @@ -129,12 +129,14 @@ void kcsan_report_set_info(const volatile void *ptr, size_t size, int access_typ * thread. */ void kcsan_report_known_origin(const volatile void *ptr, size_t size, int access_type, - enum kcsan_value_change value_change, int watchpoint_idx); + enum kcsan_value_change value_change, int watchpoint_idx, + u64 old, u64 new, u64 mask); /* * No other thread was observed to race with the access, but the data value * before and after the stall differs. Reports a race of "unknown origin". */ -void kcsan_report_unknown_origin(const volatile void *ptr, size_t size, int access_type); +void kcsan_report_unknown_origin(const volatile void *ptr, size_t size, int access_type, + u64 old, u64 new, u64 mask); #endif /* _KERNEL_KCSAN_KCSAN_H */ diff --git a/kernel/kcsan/report.c b/kernel/kcsan/report.c index 50cee2357885..e37e4386f86d 100644 --- a/kernel/kcsan/report.c +++ b/kernel/kcsan/report.c @@ -327,7 +327,8 @@ static void print_verbose_info(struct task_struct *task) static void print_report(enum kcsan_value_change value_change, const struct access_info *ai, - const struct other_info *other_info) + const struct other_info *other_info, + u64 old, u64 new, u64 mask) { unsigned long stack_entries[NUM_STACK_ENTRIES] = { 0 }; int num_stack_entries = stack_trace_save(stack_entries, NUM_STACK_ENTRIES, 1); @@ -407,6 +408,24 @@ static void print_report(enum kcsan_value_change value_change, if (IS_ENABLED(CONFIG_KCSAN_VERBOSE)) print_verbose_info(current); + /* Print observed value change. */ + if (ai->size <= 8) { + int hex_len = ai->size * 2; + u64 diff = old ^ new; + + if (mask) + diff &= mask; + if (diff) { + pr_err("\n"); + pr_err("value changed: 0x%0*llx -> 0x%0*llx\n", + hex_len, old, hex_len, new); + if (mask) { + pr_err(" bits changed: 0x%0*llx with mask 0x%0*llx\n", + hex_len, diff, hex_len, mask); + } + } + } + /* Print report footer. */ pr_err("\n"); pr_err("Reported by Kernel Concurrency Sanitizer on:\n"); @@ -584,7 +603,8 @@ void kcsan_report_set_info(const volatile void *ptr, size_t size, int access_typ } void kcsan_report_known_origin(const volatile void *ptr, size_t size, int access_type, - enum kcsan_value_change value_change, int watchpoint_idx) + enum kcsan_value_change value_change, int watchpoint_idx, + u64 old, u64 new, u64 mask) { const struct access_info ai = prepare_access_info(ptr, size, access_type); struct other_info *other_info = &other_infos[watchpoint_idx]; @@ -608,7 +628,7 @@ void kcsan_report_known_origin(const volatile void *ptr, size_t size, int access * be done once we know the full stack trace in print_report(). */ if (value_change != KCSAN_VALUE_CHANGE_FALSE) - print_report(value_change, &ai, other_info); + print_report(value_change, &ai, other_info, old, new, mask); release_report(&flags, other_info); out: @@ -616,7 +636,8 @@ out: kcsan_enable_current(); } -void kcsan_report_unknown_origin(const volatile void *ptr, size_t size, int access_type) +void kcsan_report_unknown_origin(const volatile void *ptr, size_t size, int access_type, + u64 old, u64 new, u64 mask) { const struct access_info ai = prepare_access_info(ptr, size, access_type); unsigned long flags; @@ -625,7 +646,7 @@ void kcsan_report_unknown_origin(const volatile void *ptr, size_t size, int acce lockdep_off(); /* See kcsan_report_known_origin(). */ raw_spin_lock_irqsave(&report_lock, flags); - print_report(KCSAN_VALUE_CHANGE_TRUE, &ai, NULL); + print_report(KCSAN_VALUE_CHANGE_TRUE, &ai, NULL, old, new, mask); raw_spin_unlock_irqrestore(&report_lock, flags); lockdep_on(); -- cgit v1.2.3