lkml.org 
[lkml]   [2013]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH] tracing: Atomically get refcounts of event_call and trace_array
From
Date
Sorry for the top post. FYI today is a US holiday and tomorrow I'll probably only work a half day. I may not get a chance to look it till Monday.

-- Steve


Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

>Steven, Oleg,
>
>I think your patches are OK, but not enough.
>Here is an additional patch to fix the unsafe case which I found.
>Could you review this too?
>
>(2013/07/04 20:55), Masami Hiramatsu wrote:
>> Currently ftrace_open_generic_file gets an event_file from
>> inode->i_private, and then locks event_mutex and gets refcount.
>> However, this can cause a race as below scenario;
>>
>> CPU0 CPU1
>> open(kprobe_events)
>> trace_remove_event_call() open(enable)
>> lock event_mutex get event_file from inode->i_private
>> 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 and we
>> may access the corrupted object.
>> The same thing could happen on trace_array because it is also
>> directly accessed from event_file.
>>
>> To avoid this, when opening events/*/*/enable, we must atomically
>> do; ensure the ftrace_event_file object still exists on a
>trace_array,
>> and get refcounts of event_file->call and the trace_array.
>>
>>
>> CPU0 CPU1
>> open(kprobe_events)
>> trace_remove_event_call() open(enable)
>> lock event_mutex get event_file from inode->i_private
>> event_remove() wait for unlock event_mutex
>> ...
>> free event_file
>> unlock event_mutex
>> lock event_mutex
>> search the event_file and failed
>> unlock event_mutex
>> return -ENODEV
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> ---
>> kernel/trace/trace_events.c | 58
>+++++++++++++++++++++++++++++++++----------
>> 1 file changed, 45 insertions(+), 13 deletions(-)
>>
>> diff --git a/kernel/trace/trace_events.c
>b/kernel/trace/trace_events.c
>> index 1a5547e..db6b107 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -391,15 +391,24 @@ 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)
>> +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++;
>> +
>> + return ret;
>> +}
>> +
>> +static int ftrace_event_call_get(struct ftrace_event_call *call)
>> +{
>> + int ret = 0;
>> +
>> + mutex_lock(&event_mutex);
>> + ret = __ftrace_event_call_get(call);
>> mutex_unlock(&event_mutex);
>>
>> return ret;
>> @@ -413,6 +422,35 @@ static void ftrace_event_call_put(struct
>ftrace_event_call *call)
>> mutex_unlock(&event_mutex);
>> }
>>
>> +static int ftrace_event_file_get(struct ftrace_event_file
>*this_file)
>> +{
>> + struct ftrace_event_file *file;
>> + struct trace_array *tr;
>> + int ret = -ENODEV;
>> +
>> + mutex_lock(&event_mutex);
>> + do_for_each_event_file(tr, file) {
>> + if (file == this_file) {
>> + ret = __ftrace_event_call_get(file->event_call);
>> + if (!ret)
>> + tr->ref++;
>> + goto out_unlock;
>> + }
>> + } while_for_each_event_file();
>> + out_unlock:
>> + mutex_unlock(&event_mutex);
>> +
>> + return ret;
>> +}
>> +
>> +static void ftrace_event_file_put(struct ftrace_event_file *file)
>> +{
>> + struct trace_array *tr = file->tr;
>> +
>> + ftrace_event_call_put(file->event_call);
>> + trace_array_put(tr);
>> +}
>> +
>> static void __put_system_dir(struct ftrace_subsystem_dir *dir)
>> {
>> WARN_ON_ONCE(dir->ref_count == 0);
>> @@ -438,33 +476,27 @@ static void put_system(struct
>ftrace_subsystem_dir *dir)
>> static int tracing_open_generic_file(struct inode *inode, struct
>file *filp)
>> {
>> struct ftrace_event_file *file = inode->i_private;
>> - struct trace_array *tr = file->tr;
>> int ret;
>>
>> - if (trace_array_get(tr) < 0)
>> - return -ENODEV;
>> -
>> - ret = tracing_open_generic(inode, filp);
>> + ret = ftrace_event_file_get(file);
>> if (ret < 0)
>> - goto fail;
>> + return ret;
>>
>> - ret = ftrace_event_call_get(file->event_call);
>> + ret = tracing_open_generic(inode, filp);
>> if (ret < 0)
>> goto fail;
>>
>> return 0;
>> fail:
>> - trace_array_put(tr);
>> + ftrace_event_file_put(file);
>> return ret;
>> }
>>
>> 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);
>> + ftrace_event_file_put(file);
>>
>> return 0;
>> }

--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.


\
 
 \ /
  Last update: 2013-07-04 15:21    [W:2.617 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site