Re: [PATCH] tracing: fix referencing after memory freeing andrefactors code

From: Geyslan GregÃrio Bem
Date: Fri Oct 18 2013 - 07:02:38 EST


2013/10/18 Geyslan GregÃrio Bem <geyslan@xxxxxxxxx>:
> 2013/10/17 Steven Rostedt <rostedt@xxxxxxxxxxx>:
>> On Thu, 17 Oct 2013 22:44:56 -0300
>> "Geyslan G. Bem" <geyslan@xxxxxxxxx> wrote:
>>
>>> Restructures function logic conditions testing 'tracing_open_generic'
>>> return before the others. It avoids: unnecessary trace_array_get and
>>> kzalloc when tracing is disabled; and fix the possible 'dir'
>>> assignment after freeing it.
>>>
>>> Centralizes the exiting, ensuring the 'trace_array_put' on error,
>>> in accordance to Coding Style, Chapter 7.
>>>
>>> Signed-off-by: Geyslan G. Bem <geyslan@xxxxxxxxx>
>>> ---
>>> kernel/trace/trace_events.c | 22 ++++++++++++----------
>>> 1 file changed, 12 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
>>> index 368a4d5..8232362 100644
>>> --- a/kernel/trace/trace_events.c
>>> +++ b/kernel/trace/trace_events.c
>>> @@ -1108,26 +1108,28 @@ static int system_tr_open(struct inode *inode, struct file *filp)
>>> struct trace_array *tr = inode->i_private;
>>> int ret;
>>>
>>> - if (trace_array_get(tr) < 0)
>>> - return -ENODEV;
>>> + ret = tracing_open_generic(inode, filp);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = trace_array_get(tr);
>>> + if (ret)
>>> + return ret;
>>
>> Hmm,
>>
>> I'm thinking of just nuking the tracing_open_generic() here. The only
>> thing it does here is the tracing_disabled check. The assignment of
>> inode->i_private to filp->private_data is pointless as it just
>> reassigns it anyway.
>>
>> We could add a tracing_is_disabled() function to test instead.
>
> Nice, I can do it.
>
> Questions:

I realized that here not:
>
> 1267 static const struct file_operations ftrace_enable_fops = {
> 1268 .open = tracing_open_generic,
> ...
> 1286 static const struct file_operations ftrace_event_filter_fops = {
> 1287 .open = tracing_open_generic,
> ...
> 1317 static const struct file_operations ftrace_show_header_fops = {
> 1318 .open = tracing_open_generic,

Tell me about this other case:
>
> Are that structures in same case? Their 'open' can be replaced to the
> new 'tracing_is_disabled()?
>
> I think that 'subsystem_open()' can be also refactored to use the
> about to rise 'tracing_is_disabled()'. Am I right?
> 1096 ret = tracing_open_generic(inode, filp);

Regards,
-- Geyslan
>
>>
>> -- Steve
>>
>>>
>>> /* Make a temporary dir that has no system but points to tr */
>>> dir = kzalloc(sizeof(*dir), GFP_KERNEL);
>>> if (!dir) {
>>> - trace_array_put(tr);
>>> - return -ENOMEM;
>>> + ret = -ENOMEM;
>>> + goto err_dir;
>>> }
>>>
>>> dir->tr = tr;
>>>
>>> - ret = tracing_open_generic(inode, filp);
>>> - if (ret < 0) {
>>> - trace_array_put(tr);
>>> - kfree(dir);
>>> - }
>>> -
>>> filp->private_data = dir;
>>> + return 0;
>>>
>>> +err_dir:
>>> + trace_array_put(tr);
>>> return ret;
>>> }
>>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/