* Marco Elver <[email protected]> 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 <[email protected]>
> ---
>
>
> 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?).
>
> 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
Please, make such type of assignment blocks cleaner by using a local
helper variable, and by aligning the right side vertically as well.
Also, would it make sense to unify the layout between the fields in
task struct and the new one you introduced? That would allow a simple
structure copy.
Thanks,
Ingo
