linux-kernel - Re: [PATCH tip/locking/core] kcsan: Improve IRQ state trace reporting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CANpmjNOMQ09N4+W9Dt53j=GKAuj0Sd__agRtqpDkubZEkULAJg@mail.gmail.com>
Date:   Tue, 28 Jul 2020 17:14:21 +0200
From:   Marco Elver <elver@...gle.com>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Borislav Petkov <bp@...en8.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        kasan-dev <kasan-dev@...glegroups.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH tip/locking/core] kcsan: Improve IRQ state trace reporting

On Tue, 28 Jul 2020 at 13:30, Ingo Molnar <mingo@...nel.org> wrote:
>
>
> * Marco Elver <elver@...gle.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@...gle.com>
> > ---
[...]
> > +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.

Makes sense, thanks for the suggestion. I think we could introduce a
new struct 'irqtrace_events'. I currently have something that adds
this struct in <linux/irqtrace.h>. AFAIK it also adds readability
improvements on initialization and use of the fields. I'll send a v2
with 2 patches.

Thanks,
-- Marco