linux-kernel - Re: [PATCH ftrace/core] tracing: irqsoff: Account for additional preempt

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]

Date:   Mon, 6 Aug 2018 07:14:35 -0700
From:   Joel Fernandes <joelaf@...gle.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     "Joel Fernandes (Google)" <joel@...lfernandes.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "Cc: Android Kernel" <kernel-team@...roid.com>,
        Ingo Molnar <mingo@...hat.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Paul McKenney <paulmck@...ux.vnet.ibm.com>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH ftrace/core] tracing: irqsoff: Account for additional preempt_disable

On Mon, Aug 6, 2018 at 7:05 AM, Masami Hiramatsu <mhiramat@...nel.org> wrote:
> On Sun,  5 Aug 2018 20:40:49 -0700
> "Joel Fernandes (Google)" <joel@...lfernandes.org> wrote:
>
>> Recently we tried to make the preemptirqsoff tracer to use irqsoff
>> tracepoint probes. However this causes issues as reported by Masami:
>>
>> [2.271078] Testing tracer preemptirqsoff: .. no entries found ..FAILED!
>> [2.381015] WARNING: CPU: 0 PID: 1 at /home/mhiramat/ksrc/linux/kernel/
>> trace/trace.c:1512 run_tracer_selftest+0xf3/0x154
>>
>> This is due to the tracepoint code increasing the preempt nesting count
>> by calling an additional preempt_disable before calling into the
>> preemptoff tracer which messes up the preempt_count() check in
>> tracer_hardirqs_off.
>>
>> To fix this, make the irqsoff tracer probes balance the additional outer
>> preempt_disable with a preempt_enable_notrace.
>
> I've tested it and ensured this fixes the problem.
>
> Tested-by: Masami Hiramatsu <mhiramat@...nel.org>

Thanks!

>> The other way to fix this is to just use SRCU for all tracepoints.
>> However we can't do that because we can't use NMIs from RCU context.
>>
>> Fixes: c3bc8fd637a9 ("tracing: Centralize preemptirq tracepoints
>>                       and unify their usage")
>> Fixes: e6753f23d961 ("tracepoint: Make rcuidle tracepoint callers use SRCU")
>> Reported-by: Masami Hiramatsu <mhiramat@...nel.org>
>> Signed-off-by: Joel Fernandes (Google) <joel@...lfernandes.org>
>> ---
>>  kernel/trace/trace_irqsoff.c | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c
>> index 770cd30cda40..ffbf1505d5bc 100644
>> --- a/kernel/trace/trace_irqsoff.c
>> +++ b/kernel/trace/trace_irqsoff.c
>> @@ -603,14 +603,40 @@ static void irqsoff_tracer_stop(struct trace_array *tr)
>>   */
>>  static void tracer_hardirqs_on(void *none, unsigned long a0, unsigned long a1)
>>  {
>
> To ensure this function must not be preempted even if we increment preempt
> count, I think you should check irq_disabled() whole this process, put below
> here.
>
>         if (unlikely(!irq_disabled()))
>                 return;
>
> Since irq_disabled() will be checked in irq_trace() anyway, so no problem
> to return here when !irq_disabled().

IRQs can never be enabled here. The trace hooks are called only after
disabling interrupts, or before enabling them. Right?

thanks,

- Joel