On Thu, Aug 06, 2020 at 01:32PM +0200, pet...@infradead.org wrote: > On Thu, Aug 06, 2020 at 09:47:23AM +0200, Marco Elver wrote: > > Testing my hypothesis that raw then nested non-raw > > local_irq_save/restore() breaks IRQ state tracking -- see the reproducer > > below. This is at least 1 case I can think of that we're bound to hit. ... > > /me goes ponder things... > > How's something like this then? > > --- > include/linux/sched.h | 3 --- > kernel/kcsan/core.c | 62 > ++++++++++++++++++++++++++++++++++++--------------- > 2 files changed, 44 insertions(+), 21 deletions(-)
Thank you! That approach seems to pass syzbot (also with CONFIG_PARAVIRT) and kcsan-test tests. I had to modify it some, so that report.c's use of the restore logic works and not mess up the IRQ trace printed on KCSAN reports (with CONFIG_KCSAN_VERBOSE). I still need to fully convince myself all is well now and we don't end up with more fixes. :-) If it passes further testing, I'll send it as a real patch (I want to add you as Co-developed-by, but would need your Signed-off-by for the code you pasted, I think.) Thanks, -- Marco ------ >8 ------ diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c index 9147ff6a12e5..b1d5dca10aa5 100644 --- a/kernel/kcsan/core.c +++ b/kernel/kcsan/core.c @@ -4,6 +4,7 @@ #include <linux/bug.h> #include <linux/delay.h> #include <linux/export.h> +#include <linux/ftrace.h> #include <linux/init.h> #include <linux/kernel.h> #include <linux/list.h> @@ -291,13 +292,28 @@ static inline unsigned int get_delay(void) 0); } -void kcsan_save_irqtrace(struct task_struct *task) -{ +/* + * KCSAN instrumentation is everywhere, which means we must treat the hooks + * NMI-like for interrupt tracing. In order to present a 'normal' as possible + * context to the code called by KCSAN when reporting errors we need to update + * the IRQ-tracing state. + * + * Save and restore the IRQ state trace touched by KCSAN, since KCSAN's + * runtime is entered for every memory access, and potentially useful + * information is lost if dirtied by KCSAN. + */ + +struct kcsan_irq_state { + unsigned long flags; #ifdef CONFIG_TRACE_IRQFLAGS - task->kcsan_save_irqtrace = task->irqtrace; + int hardirqs; #endif -} +}; +/* + * This is also called by the reporting task for the other task, to generate the + * right report with CONFIG_KCSAN_VERBOSE. No harm in restoring more than once. + */ void kcsan_restore_irqtrace(struct task_struct *task) { #ifdef CONFIG_TRACE_IRQFLAGS @@ -305,6 +321,34 @@ void kcsan_restore_irqtrace(struct task_struct *task) #endif } +static void kcsan_irq_save(struct kcsan_irq_state *irq_state) { +#ifdef CONFIG_TRACE_IRQFLAGS + current->kcsan_save_irqtrace = current->irqtrace; + irq_state->hardirqs = lockdep_hardirqs_enabled(); +#endif + if (!kcsan_interrupt_watcher) { + raw_local_irq_save(irq_state->flags); + kcsan_disable_current(); /* Lockdep might WARN. */ + lockdep_hardirqs_off(CALLER_ADDR0); + kcsan_enable_current(); + } +} + +static void kcsan_irq_restore(struct kcsan_irq_state *irq_state) { + if (!kcsan_interrupt_watcher) { +#ifdef CONFIG_TRACE_IRQFLAGS + if (irq_state->hardirqs) { + kcsan_disable_current(); /* Lockdep might WARN. */ + lockdep_hardirqs_on_prepare(CALLER_ADDR0); + lockdep_hardirqs_on(CALLER_ADDR0); + kcsan_enable_current(); + } +#endif + raw_local_irq_restore(irq_state->flags); + } + kcsan_restore_irqtrace(current); +} + /* * Pull everything together: check_access() below contains the performance * critical operations; the fast-path (including check_access) functions should @@ -350,11 +394,13 @@ static noinline void kcsan_found_watchpoint(const volatile void *ptr, flags = user_access_save(); if (consumed) { - kcsan_save_irqtrace(current); + struct kcsan_irq_state irqstate; + + kcsan_irq_save(&irqstate); kcsan_report(ptr, size, type, KCSAN_VALUE_CHANGE_MAYBE, KCSAN_REPORT_CONSUMED_WATCHPOINT, watchpoint - watchpoints); - kcsan_restore_irqtrace(current); + kcsan_irq_restore(&irqstate); } else { /* * The other thread may not print any diagnostics, as it has @@ -387,7 +433,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(); - unsigned long irq_flags = 0; + struct kcsan_irq_state irqstate; /* * Always reset kcsan_skip counter in slow-path to avoid underflow; see @@ -412,14 +458,7 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type) goto out; } - /* - * Save and restore the IRQ state trace touched by KCSAN, since KCSAN's - * runtime is entered for every memory access, and potentially useful - * information is lost if dirtied by KCSAN. - */ - kcsan_save_irqtrace(current); - if (!kcsan_interrupt_watcher) - local_irq_save(irq_flags); + kcsan_irq_save(&irqstate); watchpoint = insert_watchpoint((unsigned long)ptr, size, is_write); if (watchpoint == NULL) { @@ -559,9 +598,7 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type) remove_watchpoint(watchpoint); kcsan_counter_dec(KCSAN_COUNTER_USED_WATCHPOINTS); out_unlock: - if (!kcsan_interrupt_watcher) - local_irq_restore(irq_flags); - kcsan_restore_irqtrace(current); + kcsan_irq_restore(&irqstate); out: user_access_restore(ua_flags); } diff --git a/kernel/kcsan/kcsan.h b/kernel/kcsan/kcsan.h index 29480010dc30..6eb35a9514d8 100644 --- a/kernel/kcsan/kcsan.h +++ b/kernel/kcsan/kcsan.h @@ -24,9 +24,8 @@ extern unsigned int kcsan_udelay_interrupt; extern bool kcsan_enabled; /* - * Save/restore IRQ flags state trace dirtied by KCSAN. + * Restore IRQ flags state trace dirtied by KCSAN. */ -void kcsan_save_irqtrace(struct task_struct *task); void kcsan_restore_irqtrace(struct task_struct *task); /*