linux-kernel - Re: [PATCH] tracing: Prevent RCU EQS breakage in preemptirq 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 for Android: free password hash cracker in your pocket

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

Message-ID: <CALCETrVavLdQ8Rp+6fmTd7kJJwvRKdaEnudaiMAu8g9ZXuNfWA@mail.gmail.com>
Date:   Sun, 28 Jul 2019 21:25:58 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Eiichi Tsukata <devel@...ukata.com>
Cc:     Joel Fernandes <joel@...lfernandes.org>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Frederic Weisbecker <fweisbec@...il.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] tracing: Prevent RCU EQS breakage in preemptirq events

On Sun, Jul 28, 2019 at 6:08 PM Eiichi Tsukata <devel@...ukata.com> wrote:
>
> If context tracking is enabled, causing page fault in preemptirq
> irq_enable or irq_disable events triggers the following RCU EQS warning.
>

Yuck.

> diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
> index be01a4d627c9..860eaf9780e5 100644
> --- a/kernel/context_tracking.c
> +++ b/kernel/context_tracking.c
> @@ -148,6 +148,11 @@ void __context_tracking_exit(enum ctx_state state)
>                 return;
>
>         if (__this_cpu_read(context_tracking.state) == state) {
> +               /*
> +                * Change state before executing codes which can trigger
> +                * page fault leading unnecessary re-entrance.
> +                */
> +               __this_cpu_write(context_tracking.state, CONTEXT_KERNEL);

Seems reasonable.

>                 if (__this_cpu_read(context_tracking.active)) {
>                         /*
>                          * We are going to run code that may use RCU. Inform
> @@ -159,7 +164,6 @@ void __context_tracking_exit(enum ctx_state state)
>                                 trace_user_exit(0);
>                         }
>                 }
> -               __this_cpu_write(context_tracking.state, CONTEXT_KERNEL);
>         }
>         context_tracking_recursion_exit();
>  }
> diff --git a/kernel/trace/trace_preemptirq.c b/kernel/trace/trace_preemptirq.c
> index 4d8e99fdbbbe..031b51cb94d0 100644
> --- a/kernel/trace/trace_preemptirq.c
> +++ b/kernel/trace/trace_preemptirq.c
> @@ -10,6 +10,7 @@
>  #include <linux/module.h>
>  #include <linux/ftrace.h>
>  #include <linux/kprobes.h>
> +#include <linux/context_tracking.h>
>  #include "trace.h"
>
>  #define CREATE_TRACE_POINTS
> @@ -49,9 +50,14 @@ NOKPROBE_SYMBOL(trace_hardirqs_off);
>
>  __visible void trace_hardirqs_on_caller(unsigned long caller_addr)
>  {
> +       enum ctx_state prev_state;
> +
>         if (this_cpu_read(tracing_irq_cpu)) {
> -               if (!in_nmi())
> +               if (!in_nmi()) {
> +                       prev_state = exception_enter();
>                         trace_irq_enable_rcuidle(CALLER_ADDR0, caller_addr);
> +                       exception_exit(prev_state);
> +               }
>                 tracer_hardirqs_on(CALLER_ADDR0, caller_addr);
>                 this_cpu_write(tracing_irq_cpu, 0);
>         }

This seems a bit distressing.  Now we're going to do a whole bunch of
context tracking transitions for each kernel entry.  Would a better
fix me to change trace_hardirqs_on_caller to skip the trace event if
the previous state was already IRQs on and, more importantly, to skip
tracing IRQs off if IRQs were already off?  The x86 code is very
careful to avoid ever having IRQs on and CONTEXT_USER at the same
time.

--Andy