linux-kernel - Re: [PATCH v6 2/2] tracing: Add support for preempt and irq enable/disable events

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJWu+or=5KYvg0nXJmB=_ksJyYpZJ4GzMvXu64xqCfn0-9NGgg@mail.gmail.com>
Date:   Mon, 2 Oct 2017 12:23:51 -0700
From:   Joel Fernandes <joelaf@...gle.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>, kernel-team@...roid.com
Subject: Re: [PATCH v6 2/2] tracing: Add support for preempt and irq
 enable/disable events

Hi Steven,

On Sat, Sep 30, 2017 at 2:08 AM, Steven Rostedt <rostedt@...dmis.org> wrote:
> On Mon, 25 Sep 2017 17:23:00 -0700
> Joel Fernandes <joelaf@...gle.com> wrote:
>
>> The trace_hardirqs_off API can be called even when IRQs are already
>> off. This is unlike the trace_hardirqs_on which checks if IRQs are off
>> (atleast from some callsites), here are the definitions just for
>> reference [1]. I guess we could modify local_irq_disable and
>> local_irq_save to check what the HW flags was before calling
>> raw_local_irq_save and only then call trace_hardirqs_off if they were
>> indeed on and now being turned off, but that adds complexity to it -
>> also we have to then modify all the callsites from assembly code to
>> conditionally call trace_hardirqs_on/off :(.
>>
>
>> [1] http://elixir.free-electrons.com/linux/latest/source/include/linux/irqflags.h#L89
>
> Yeah, I think the issue for the recursion is basically this:
>
> #define local_irq_restore(flags)                        \
>         do {                                            \
>                 if (raw_irqs_disabled_flags(flags)) {   \
>                         raw_local_irq_restore(flags);   \
>                         trace_hardirqs_off();           \
>                 } else {                                \
>                         trace_hardirqs_on();            \
>                         raw_local_irq_restore(flags);   \
>                 }                                       \
>         } while (0)
>
>
> Peter,
>
> Is there any place where we would call local_irq_restore() when
> interrupts are enabled?

Actually local_irq_restore doesn't have the problem I describe since
we do check for 'flags' and based on the condition and *only* then
call the trace_hardirqs_* functions right?

The problem I describe is the following case where trace_hardirqs_off
can be called twice, this is just one example:
The APIC timer interrupt is received and trace_hardirqs_off_caller is
called. So we register that irqs just got turned off.
Then, some time in the interrupt exit path, it calls local_irq_disable
again, this can happen in this path:

irq_exit ->  local_irq_disable -> trace_hardirqs_off

This can happen if __ARCH_IRQ_EXIT_IRQS_DISABLED != 1.

The other path this can happen from is: ret_from_intr ->
TRACE_IRQS_OFF while interrupts are already off.

For now, can we just use the per-CPU variable as I did in my latest
patchset to avoid the double calling for the the IRQ disable cases? Do
you have or Peter have any issues with that?
https://patchwork.kernel.org/patch/9978705/
I documented the need for the per-CPU variable in that patch as Peter
wanted. Please let me know if the patch I linked above looks Ok,
thanks a lot.

thanks,

- Joel