From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-kernel@vger.kernel.org, Oleg Nesterov <oleg@redhat.com>,
"zhangwei(Jovi)" <jovi.zhangwei@huawei.com>,
Jiri Olsa <jolsa@redhat.com>,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
Arnaldo Carvalho de Melo <acme@ghostprotocols.net>,
Srikar Dronamraju <srikar@linux.vnet.ibm.com>,
Frederic Weisbecker <fweisbec@gmail.com>,
Ingo Molnar <mingo@redhat.com>,
Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [RFC][PATCH 1/4] tracing: Add ref count to ftrace_event_call
Date: Thu, 04 Jul 2013 13:22:29 +0900 [thread overview]
Message-ID: <51D4F885.3040105@hitachi.com> (raw)
In-Reply-To: <20130704034038.501144114@goodmis.org>
(2013/07/04 12:33), Steven Rostedt wrote:
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 7d85429..90cf243 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -391,6 +391,28 @@ static void __get_system_dir(struct ftrace_subsystem_dir *dir)
> __get_system(dir->subsystem);
> }
>
> +static int ftrace_event_call_get(struct ftrace_event_call *call)
> +{
> + int ret = 0;
> +
> + mutex_lock(&event_mutex);
> + if ((call->flags & TRACE_EVENT_FL_REF_MASK) == TRACE_EVENT_FL_REF_MAX - 1)
> + ret = -EBUSY;
> + else
> + call->flags++;
> + mutex_unlock(&event_mutex);
> +
> + return ret;
> +}
> +
> +static void ftrace_event_call_put(struct ftrace_event_call *call)
> +{
> + mutex_lock(&event_mutex);
> + if (!WARN_ON_ONCE(!(call->flags & TRACE_EVENT_FL_REF_MASK)))
> + call->flags--;
> + mutex_unlock(&event_mutex);
> +}
Hmm, I might misunderstand, but it seems that there is a small unsafe
time slot.
> @@ -424,7 +446,15 @@ static int tracing_open_generic_file(struct inode *inode, struct file *filp)
>
> ret = tracing_open_generic(inode, filp);
> if (ret < 0)
> - trace_array_put(tr);
> + goto fail;
> +
> + ret = ftrace_event_call_get(file->event_call);
> + if (ret < 0)
> + goto fail;
> +
> + return 0;
> + fail:
> + trace_array_put(tr);
> return ret;
> }
>
> @@ -433,12 +463,40 @@ static int tracing_release_generic_file(struct inode *inode, struct file *filp)
> struct ftrace_event_file *file = inode->i_private;
> struct trace_array *tr = file->tr;
>
> + ftrace_event_call_put(file->event_call);
> trace_array_put(tr);
>
> return 0;
> }
Here, at first we get an event_file from inode->i_private, and then locks
event_mutex and get/put refcount. This should be safe if we get (find) the object
from some list of event_file under the mutex, but we just use inode->i_private.
This can cause a race as below scenario,
CPU0 CPU1
open(kprobe_events)
trace_remove_event_call() open(enable)
lock event_mutex refer event_file
event_remove() wait for unlock event_mutex
...
free event_file
unlock event_mutex
lock event_mutex
add refcount of event_file->call (*)
So, at (*) point, the event_file is already freed. Thus there still be
an unsafe time slot. Or, did I miss something (possibly..)?
Thank you,
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com
next prev parent reply other threads:[~2013-07-04 4:22 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-04 3:33 [RFC][PATCH 0/4] tracing/kprobes/uprobes: Fix race between opening probe event files and deleting probe Steven Rostedt
2013-07-04 3:33 ` [RFC][PATCH 1/4] tracing: Add ref count to ftrace_event_call Steven Rostedt
2013-07-04 4:22 ` Masami Hiramatsu [this message]
2013-07-04 11:55 ` [RFC PATCH] tracing: Atomically get refcounts of event_call and trace_array Masami Hiramatsu
2013-07-04 12:12 ` Masami Hiramatsu
2013-07-04 12:23 ` Steven Rostedt
2013-07-05 0:32 ` Oleg Nesterov
2013-07-05 2:32 ` Masami Hiramatsu
2013-07-09 7:55 ` [RFC PATCH V2] tracing: Check f_dentry before accessing event_file/call in inode->i_private Masami Hiramatsu
2013-07-15 18:16 ` Oleg Nesterov
2013-07-17 2:10 ` Masami Hiramatsu
2013-07-17 14:51 ` Oleg Nesterov
2013-07-18 2:20 ` Masami Hiramatsu
2013-07-18 14:51 ` Oleg Nesterov
2013-07-19 5:21 ` Masami Hiramatsu
2013-07-19 13:33 ` Oleg Nesterov
2013-07-22 9:57 ` Masami Hiramatsu
2013-07-22 17:04 ` Oleg Nesterov
2013-07-23 21:04 ` Oleg Nesterov
2013-07-04 3:33 ` [RFC][PATCH 2/4] tracing: trace_remove_event_call() should fail if call/file is in use Steven Rostedt
2013-07-04 12:48 ` Masami Hiramatsu
2013-07-04 3:33 ` [RFC][PATCH 3/4] tracing/kprobes: Fail to unregister if probe event files are open Steven Rostedt
2013-07-04 12:45 ` Masami Hiramatsu
2013-07-04 18:48 ` Oleg Nesterov
2013-07-05 2:53 ` Masami Hiramatsu
2013-07-05 17:26 ` Oleg Nesterov
2013-07-08 2:36 ` Masami Hiramatsu
2013-07-08 14:25 ` Oleg Nesterov
2013-07-09 8:01 ` [RFC PATCH] tracing/kprobe: Wait for disabling all running kprobe handlers Masami Hiramatsu
2013-07-09 8:07 ` Peter Zijlstra
2013-07-09 8:20 ` Masami Hiramatsu
2013-07-09 8:21 ` Peter Zijlstra
2013-07-09 8:50 ` Masami Hiramatsu
2013-07-09 9:35 ` [RFC PATCH V2] " Masami Hiramatsu
2013-07-15 18:20 ` Oleg Nesterov
2013-07-18 12:07 ` Masami Hiramatsu
2013-07-18 14:35 ` Steven Rostedt
2013-07-30 8:15 ` [RFC][PATCH 3/4] tracing/kprobes: Fail to unregister if probe event files are open Masami Hiramatsu
2013-07-31 19:49 ` Steven Rostedt
2013-07-31 20:40 ` Oleg Nesterov
2013-07-31 22:42 ` Steven Rostedt
2013-08-01 2:07 ` Steven Rostedt
2013-08-01 2:50 ` Steven Rostedt
2013-08-01 3:48 ` Masami Hiramatsu
2013-08-01 13:34 ` Oleg Nesterov
2013-08-01 13:49 ` Oleg Nesterov
2013-08-01 14:17 ` Steven Rostedt
2013-08-01 14:33 ` Oleg Nesterov
2013-08-01 14:45 ` Steven Rostedt
2013-08-01 14:46 ` Oleg Nesterov
2013-08-02 4:57 ` Masami Hiramatsu
2013-08-01 13:10 ` Oleg Nesterov
2013-07-04 3:33 ` [RFC][PATCH 4/4] tracing/uprobes: " Steven Rostedt
2013-08-01 3:40 ` Steven Rostedt
2013-08-01 14:08 ` Oleg Nesterov
2013-08-01 14:25 ` Steven Rostedt
2013-07-04 4:00 ` [RFC][PATCH 0/4] tracing/kprobes/uprobes: Fix race between opening probe event files and deleting probe Masami Hiramatsu
2013-07-04 6:14 ` Masami Hiramatsu
2013-07-12 13:09 ` Masami Hiramatsu
2013-07-12 17:53 ` Oleg Nesterov
2013-07-15 18:01 ` Oleg Nesterov
2013-07-16 16:38 ` Oleg Nesterov
2013-07-16 19:10 ` Steven Rostedt
2013-07-16 19:22 ` Oleg Nesterov
2013-07-16 19:38 ` Steven Rostedt
2013-07-17 16:03 ` Steven Rostedt
2013-07-17 17:37 ` Oleg Nesterov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=51D4F885.3040105@hitachi.com \
--to=masami.hiramatsu.pt@hitachi.com \
--cc=a.p.zijlstra@chello.nl \
--cc=acme@ghostprotocols.net \
--cc=akpm@linux-foundation.org \
--cc=fweisbec@gmail.com \
--cc=jolsa@redhat.com \
--cc=jovi.zhangwei@huawei.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=oleg@redhat.com \
--cc=rostedt@goodmis.org \
--cc=srikar@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.