Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200720120348.2406588-1-elver@google.com>
In-Reply-To: <20200720120348.2406588-1-elver@google.com>
From:   Marco Elver <elver@google.com>
Date:   Tue, 28 Jul 2020 12:45:08 +0200
Message-ID: <CANpmjNO7M2gm5=P9Z27_BgT3i=p6=KFBbEUaKm049wtqyBMQcA@mail.gmail.com>
Subject: Re: [PATCH tip/locking/core] kcsan: Improve IRQ state trace reporting
To:     Marco Elver <elver@google.com>,
        Peter Zijlstra <peterz@infradead.org>
Cc:     Borislav Petkov <bp@alien8.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@kernel.org>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        Dmitry Vyukov <dvyukov@google.com>,
        kasan-dev <kasan-dev@googlegroups.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 20 Jul 2020 at 14:03, Marco Elver <elver@google.com> wrote:
>
> To improve the general usefulness of the IRQ state trace information
> with KCSAN enabled, save and restore the trace information when entering
> and exiting the KCSAN runtime as well as when generating a KCSAN report.
>
> Without this, reporting the IRQ state trace (whether via a KCSAN report
> or outside of KCSAN via a lockdep report) is rather useless due to
> continuously being touched by KCSAN. This is because if KCSAN is
> enabled, every instrumented memory access causes changes to IRQ state
> tracking information (either by KCSAN disabling/enabling interrupts or
> taking report_lock when generating a report).
>
> Before "lockdep: Prepare for NMI IRQ state tracking", KCSAN avoided
> touching the IRQ state trace via raw_local_irq_save/restore() and
> lockdep_off/on().
>
> Fixes: 248591f5d257 ("kcsan: Make KCSAN compatible with new IRQ state tracking")
> Signed-off-by: Marco Elver <elver@google.com>
> ---
>
>
> Hi, Peter,
>
> If this is reasonable, please take it into the branch that currently has
> the series around "lockdep: Prepare for NMI IRQ state tracking"
> (tip/locking/core?).

Just in case -- checking this one wasn't lost.

Many thanks,
-- Marco

> ---
>  include/linux/sched.h | 13 +++++++++++++
>  kernel/kcsan/core.c   | 39 +++++++++++++++++++++++++++++++++++++++
>  kernel/kcsan/kcsan.h  |  7 +++++++
>  kernel/kcsan/report.c |  3 +++
>  4 files changed, 62 insertions(+)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 692e327d7455..ca5324b1657c 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1199,6 +1199,19 @@ struct task_struct {
>  #endif
>  #ifdef CONFIG_KCSAN
>         struct kcsan_ctx                kcsan_ctx;
> +#ifdef CONFIG_TRACE_IRQFLAGS
> +       struct {
> +               unsigned int            irq_events;
> +               unsigned long           hardirq_enable_ip;
> +               unsigned long           hardirq_disable_ip;
> +               unsigned int            hardirq_enable_event;
> +               unsigned int            hardirq_disable_event;
> +               unsigned long           softirq_disable_ip;
> +               unsigned long           softirq_enable_ip;
> +               unsigned int            softirq_disable_event;
> +               unsigned int            softirq_enable_event;
> +       } kcsan_save_irqtrace;
> +#endif
>  #endif
>
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c
> index 732623c30359..7e8347c14530 100644
> --- a/kernel/kcsan/core.c
> +++ b/kernel/kcsan/core.c
> @@ -291,6 +291,36 @@ static inline unsigned int get_delay(void)
>                                 0);
>  }
>
> +void kcsan_save_irqtrace(struct task_struct *task)
> +{
> +#ifdef CONFIG_TRACE_IRQFLAGS
> +       task->kcsan_save_irqtrace.irq_events = task->irq_events;
> +       task->kcsan_save_irqtrace.hardirq_enable_ip = task->hardirq_enable_ip;
> +       task->kcsan_save_irqtrace.hardirq_disable_ip = task->hardirq_disable_ip;
> +       task->kcsan_save_irqtrace.hardirq_enable_event = task->hardirq_enable_event;
> +       task->kcsan_save_irqtrace.hardirq_disable_event = task->hardirq_disable_event;
> +       task->kcsan_save_irqtrace.softirq_disable_ip = task->softirq_disable_ip;
> +       task->kcsan_save_irqtrace.softirq_enable_ip = task->softirq_enable_ip;
> +       task->kcsan_save_irqtrace.softirq_disable_event = task->softirq_disable_event;
> +       task->kcsan_save_irqtrace.softirq_enable_event = task->softirq_enable_event;
> +#endif
> +}
> +
> +void kcsan_restore_irqtrace(struct task_struct *task)
> +{
> +#ifdef CONFIG_TRACE_IRQFLAGS
> +       task->irq_events = task->kcsan_save_irqtrace.irq_events;
> +       task->hardirq_enable_ip = task->kcsan_save_irqtrace.hardirq_enable_ip;
> +       task->hardirq_disable_ip = task->kcsan_save_irqtrace.hardirq_disable_ip;
> +       task->hardirq_enable_event = task->kcsan_save_irqtrace.hardirq_enable_event;
> +       task->hardirq_disable_event = task->kcsan_save_irqtrace.hardirq_disable_event;
> +       task->softirq_disable_ip = task->kcsan_save_irqtrace.softirq_disable_ip;
> +       task->softirq_enable_ip = task->kcsan_save_irqtrace.softirq_enable_ip;
> +       task->softirq_disable_event = task->kcsan_save_irqtrace.softirq_disable_event;
> +       task->softirq_enable_event = task->kcsan_save_irqtrace.softirq_enable_event;
> +#endif
> +}
> +
>  /*
>   * Pull everything together: check_access() below contains the performance
>   * critical operations; the fast-path (including check_access) functions should
> @@ -336,9 +366,11 @@ static noinline void kcsan_found_watchpoint(const volatile void *ptr,
>         flags = user_access_save();
>
>         if (consumed) {
> +               kcsan_save_irqtrace(current);
>                 kcsan_report(ptr, size, type, KCSAN_VALUE_CHANGE_MAYBE,
>                              KCSAN_REPORT_CONSUMED_WATCHPOINT,
>                              watchpoint - watchpoints);
> +               kcsan_restore_irqtrace(current);
>         } else {
>                 /*
>                  * The other thread may not print any diagnostics, as it has
> @@ -396,6 +428,12 @@ 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);
>
> @@ -539,6 +577,7 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type)
>  out_unlock:
>         if (!kcsan_interrupt_watcher)
>                 local_irq_restore(irq_flags);
> +       kcsan_restore_irqtrace(current);
>  out:
>         user_access_restore(ua_flags);
>  }
> diff --git a/kernel/kcsan/kcsan.h b/kernel/kcsan/kcsan.h
> index 763d6d08d94b..29480010dc30 100644
> --- a/kernel/kcsan/kcsan.h
> +++ b/kernel/kcsan/kcsan.h
> @@ -9,6 +9,7 @@
>  #define _KERNEL_KCSAN_KCSAN_H
>
>  #include <linux/kcsan.h>
> +#include <linux/sched.h>
>
>  /* The number of adjacent watchpoints to check. */
>  #define KCSAN_CHECK_ADJACENT 1
> @@ -22,6 +23,12 @@ extern unsigned int kcsan_udelay_interrupt;
>   */
>  extern bool kcsan_enabled;
>
> +/*
> + * Save/restore IRQ flags state trace dirtied by KCSAN.
> + */
> +void kcsan_save_irqtrace(struct task_struct *task);
> +void kcsan_restore_irqtrace(struct task_struct *task);
> +
>  /*
>   * Initialize debugfs file.
>   */
> diff --git a/kernel/kcsan/report.c b/kernel/kcsan/report.c
> index 6b2fb1a6d8cd..9d07e175de0f 100644
> --- a/kernel/kcsan/report.c
> +++ b/kernel/kcsan/report.c
> @@ -308,6 +308,9 @@ static void print_verbose_info(struct task_struct *task)
>         if (!task)
>                 return;
>
> +       /* Restore IRQ state trace for printing. */
> +       kcsan_restore_irqtrace(task);
> +
>         pr_err("\n");
>         debug_show_held_locks(task);
>         print_irqtrace_events(task);
> --
> 2.28.0.rc0.105.gf9edc3c819-goog
>