| 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
| ||
|
Message-ID: <20180525114339.GA30990@sejong>
Date: Fri, 25 May 2018 20:43:39 +0900
From: Namhyung Kim <namhyung@...nel.org>
To: Joel Fernandes <joelaf@...gle.com>
Cc: linux-kernel@...r.kernel.org, kernel-team@...roid.com,
"Joel Fernandes (Google)" <joel@...lfernandes.org>,
Boqun Feng <boqun.feng@...il.com>,
Byungchul Park <byungchul.park@....com>,
Erick Reyes <erickreyes@...gle.com>,
Ingo Molnar <mingo@...hat.com>,
Julia Cartwright <julia@...com>,
linux-kselftest@...r.kernel.org,
Masami Hiramatsu <mhiramat@...nel.org>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Paul McKenney <paulmck@...ux.vnet.ibm.com>,
Peter Zijlstra <peterz@...radead.org>,
Shuah Khan <shuah@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>,
Thomas Glexiner <tglx@...utronix.de>,
Todd Kjos <tkjos@...gle.com>,
Tom Zanussi <tom.zanussi@...ux.intel.com>, kernel-team@....com
Subject: Re: [PATCH v7 6/8] tracing: Centralize preemptirq tracepoints and
unify their usage
Hi Joel,
On Wed, May 23, 2018 at 06:21:55PM -0700, Joel Fernandes wrote:
> From: "Joel Fernandes (Google)" <joel@...lfernandes.org>
>
> This patch detaches the preemptirq tracepoints from the tracers and
> keeps it separate.
>
> Advantages:
> * Lockdep and irqsoff event can now run in parallel since they no longer
> have their own calls.
>
> * This unifies the usecase of adding hooks to an irqsoff and irqson
> event, and a preemptoff and preempton event.
> 3 users of the events exist:
> - Lockdep
> - irqsoff and preemptoff tracers
> - irqs and preempt trace events
>
> The unification cleans up several ifdefs and makes the code in preempt
> tracer and irqsoff tracers simpler. It gets rid of all the horrific
> ifdeferry around PROVE_LOCKING and makes configuration of the different
> users of the tracepoints more easy and understandable. It also gets rid
> of the time_* function calls from the lockdep hooks used to call into
> the preemptirq tracer which is not needed anymore. The negative delta in
> lines of code in this patch is quite large too.
>
> In the patch we introduce a new CONFIG option PREEMPTIRQ_TRACEPOINTS
> as a single point for registering probes onto the tracepoints. With
> this,
> the web of config options for preempt/irq toggle tracepoints and its
> users becomes:
>
> PREEMPT_TRACER PREEMPTIRQ_EVENTS IRQSOFF_TRACER PROVE_LOCKING
> | | \ | |
> \ (selects) / \ \ (selects) /
> TRACE_PREEMPT_TOGGLE ----> TRACE_IRQFLAGS
> \ /
> \ (depends on) /
> PREEMPTIRQ_TRACEPOINTS
>
> One note, I have to check for lockdep recursion in the code that calls
> the trace events API and bail out if we're in lockdep recursion
> protection to prevent something like the following case: a spin_lock is
> taken. Then lockdep_acquired is called. That does a raw_local_irq_save
> and then sets lockdep_recursion, and then calls __lockdep_acquired. In
> this function, a call to get_lock_stats happens which calls
> preempt_disable, which calls trace IRQS off somewhere which enters my
> tracepoint code and sets the tracing_irq_cpu flag to prevent recursion.
> This flag is then never cleared causing lockdep paths to never be
> entered and thus causing splats and other bad things.
>
> Other than the performance tests mentioned in the previous patch, I also
> ran the locking API test suite. I verified that all tests cases are
> passing.
>
> I also injected issues by not registering lockdep probes onto the
> tracepoints and I see failures to confirm that the probes are indeed
> working.
>
> Without probes:
>
> [ 0.000000] hard-irqs-on + irq-safe-A/21: ok | ok | ok |
> [ 0.000000] soft-irqs-on + irq-safe-A/21: ok | ok | ok |
> [ 0.000000] sirq-safe-A => hirqs-on/12: ok | ok | ok |
> [ 0.000000] sirq-safe-A => hirqs-on/21: ok | ok | ok |
> [ 0.000000] hard-safe-A + irqs-on/12: ok | ok | ok |
> [ 0.000000] soft-safe-A + irqs-on/12: ok | ok | ok |
> [ 0.000000] hard-safe-A + irqs-on/21: ok | ok | ok |
> [ 0.000000] soft-safe-A + irqs-on/21: ok | ok | ok |
> [ 0.000000] hard-safe-A + unsafe-B #1/123: ok | ok | ok |
> [ 0.000000] soft-safe-A + unsafe-B #1/123: ok | ok | ok |
>
> With probes:
>
> [ 0.000000] hard-irqs-on + irq-safe-A/21: ok | ok | ok |
> [ 0.000000] soft-irqs-on + irq-safe-A/21: ok | ok | ok |
> [ 0.000000] sirq-safe-A => hirqs-on/12:FAILED|FAILED| ok |
> [ 0.000000] sirq-safe-A => hirqs-on/21:FAILED|FAILED| ok |
> [ 0.000000] hard-safe-A + irqs-on/12:FAILED|FAILED| ok |
> [ 0.000000] soft-safe-A + irqs-on/12:FAILED|FAILED| ok |
> [ 0.000000] hard-safe-A + irqs-on/21:FAILED|FAILED| ok |
> [ 0.000000] soft-safe-A + irqs-on/21:FAILED|FAILED| ok |
> [ 0.000000] hard-safe-A + unsafe-B #1/123: ok | ok | ok |
> [ 0.000000] soft-safe-A + unsafe-B #1/123: ok | ok | ok |
>
> Signed-off-by: Joel Fernandes (Google) <joel@...lfernandes.org>
> ---
[SNIP]
>
> #ifdef CONFIG_IRQSOFF_TRACER
> +/*
> + * We are only interested in hardirq on/off events:
> + */
> +static void tracer_hardirqs_on(void *none, unsigned long a0, unsigned long a1)
> +{
> + if (!preempt_trace() && irq_trace())
> + stop_critical_timing(a0, a1);
> +}
> +
> +static void tracer_hardirqs_off(void *none, unsigned long a0, unsigned long a1)
> +{
> + if (!preempt_trace() && irq_trace())
> + start_critical_timing(a0, a1);
> +}
> +
> static int irqsoff_tracer_init(struct trace_array *tr)
> {
> trace_type = TRACER_IRQS_OFF;
>
> + register_trace_irq_disable(tracer_hardirqs_off, NULL);
> + register_trace_irq_enable(tracer_hardirqs_on, NULL);
> return __irqsoff_tracer_init(tr);
> }
>
> static void irqsoff_tracer_reset(struct trace_array *tr)
> {
> + unregister_trace_irq_disable(tracer_hardirqs_off, NULL);
> + unregister_trace_irq_enable(tracer_hardirqs_on, NULL);
> __irqsoff_tracer_reset(tr);
> }
>
> @@ -692,19 +650,37 @@ static struct tracer irqsoff_tracer __read_mostly =
> };
> # define register_irqsoff(trace) register_tracer(&trace)
> #else
> +static inline void tracer_hardirqs_on(unsigned long a0, unsigned long a1) { }
> +static inline void tracer_hardirqs_off(unsigned long a0, unsigned long a1) { }
Just a nitpick. These lines seem unnecessary since they're used
only when CONFIG_IRQSOFF_TRACER is enabled AFAICS.
> # define register_irqsoff(trace) do { } while (0)
> -#endif
> +#endif /* CONFIG_IRQSOFF_TRACER */
>
> #ifdef CONFIG_PREEMPT_TRACER
> +static void tracer_preempt_on(void *none, unsigned long a0, unsigned long a1)
> +{
> + if (preempt_trace() && !irq_trace())
> + stop_critical_timing(a0, a1);
> +}
> +
> +static void tracer_preempt_off(void *none, unsigned long a0, unsigned long a1)
> +{
> + if (preempt_trace() && !irq_trace())
> + start_critical_timing(a0, a1);
> +}
> +
> static int preemptoff_tracer_init(struct trace_array *tr)
> {
> trace_type = TRACER_PREEMPT_OFF;
>
> + register_trace_preempt_disable(tracer_preempt_off, NULL);
> + register_trace_preempt_enable(tracer_preempt_on, NULL);
> return __irqsoff_tracer_init(tr);
> }
>
> static void preemptoff_tracer_reset(struct trace_array *tr)
> {
> + unregister_trace_preempt_disable(tracer_preempt_off, NULL);
> + unregister_trace_preempt_enable(tracer_preempt_on, NULL);
> __irqsoff_tracer_reset(tr);
> }
>
> @@ -729,21 +705,32 @@ static struct tracer preemptoff_tracer __read_mostly =
> };
> # define register_preemptoff(trace) register_tracer(&trace)
> #else
> +static inline void tracer_preempt_on(unsigned long a0, unsigned long a1) { }
> +static inline void tracer_preempt_off(unsigned long a0, unsigned long a1) { }
Ditto (for CONFIG_PREEMPT_TRACER).
Thanks,
Namhyung
> # define register_preemptoff(trace) do { } while (0)
> -#endif
> +#endif /* CONFIG_PREEMPT_TRACER */
>
> -#if defined(CONFIG_IRQSOFF_TRACER) && \
> - defined(CONFIG_PREEMPT_TRACER)
> +#if defined(CONFIG_IRQSOFF_TRACER) && defined(CONFIG_PREEMPT_TRACER)
>
> static int preemptirqsoff_tracer_init(struct trace_array *tr)
> {
> trace_type = TRACER_IRQS_OFF | TRACER_PREEMPT_OFF;
>
> + register_trace_irq_disable(tracer_hardirqs_off, NULL);
> + register_trace_irq_enable(tracer_hardirqs_on, NULL);
> + register_trace_preempt_disable(tracer_preempt_off, NULL);
> + register_trace_preempt_enable(tracer_preempt_on, NULL);
> +
> return __irqsoff_tracer_init(tr);
> }
>
> static void preemptirqsoff_tracer_reset(struct trace_array *tr)
> {
> + unregister_trace_irq_disable(tracer_hardirqs_off, NULL);
> + unregister_trace_irq_enable(tracer_hardirqs_on, NULL);
> + unregister_trace_preempt_disable(tracer_preempt_off, NULL);
> + unregister_trace_preempt_enable(tracer_preempt_on, NULL);
> +
> __irqsoff_tracer_reset(tr);
> }
Powered by blists - more mailing lists