lkml.org 
[lkml]   [2008]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/6] tracing/ftrace: Fix a race condition in sched_switch tracer

On Fri, 10 Oct 2008, Frederic Weisbecker wrote:
>
> This patch fixes a race condition in the sched_switch tracer.
> If several tasks (IE: concurrent initcalls) are playing with
> tracing_start_cmdline_record() and tracing_stop_cmdline_record(),
> the following situation could happen:
>
>
> _ Task A and B are using the same tracepoint probe. Task A holds it. Task B is
> sleeping and doesn't hold it.
> _ Task A frees the sched tracer, then sched_ref is decremented to 0.
> _ Task A is preempted and hadn't yet unregistered its tracepoint probe,
> then B runs.
> _ B increments sched_ref, sees it's 1 and then guess it has to register its probe. But it has not been freed by task A.
> _ A lot of bad things can happen after that...

OK, I see the issue.

>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> CC: Steven Rostedt <rostedt@goodmis.org>
> ---
> kernel/trace/trace_sched_switch.c | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/trace/trace_sched_switch.c b/kernel/trace/trace_sched_switch.c
> index b8f56be..59de514 100644
> --- a/kernel/trace/trace_sched_switch.c
> +++ b/kernel/trace/trace_sched_switch.c
> @@ -17,6 +17,7 @@
> static struct trace_array *ctx_trace;
> static int __read_mostly tracer_enabled;
> static atomic_t sched_ref;
> +static DEFINE_MUTEX(tracepoint_mutex);
>
> static void
> probe_sched_switch(struct rq *__rq, struct task_struct *prev,
> @@ -125,18 +126,22 @@ static void tracing_start_sched_switch(void)
> {
> long ref;
>
> + mutex_lock(&tracepoint_mutex);
> ref = atomic_inc_return(&sched_ref);
> if (ref == 1)
> tracing_sched_register();

Could you make another patch to convert sched_ref to an int. With a mutex
lock, there's no reason for this to be atomic.

We can simply do:

if (!(sched_ref++))
tracing_sched_register();

> + mutex_unlock(&tracepoint_mutex);
> }
>
> static void tracing_stop_sched_switch(void)
> {
> long ref;
>
> + mutex_lock(&tracepoint_mutex);
> ref = atomic_dec_and_test(&sched_ref);
> if (ref)
> tracing_sched_unregister();

and

if (!(--sched_ref))
tracing_sched_unregister();

> + mutex_unlock(&tracepoint_mutex);

Also, lets change the name. sched_register_mutex?

> }
>
> void tracing_start_cmdline_record(void)
>

-- Steve




\
 
 \ /
  Last update: 2008-10-10 15:41    [W:0.029 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site