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

[Geyslan Gregório Bem]

2013/11/6 Steven Rostedt <rostedt@xxxxxxxxxxx>:
> Sorry for the late review, I was busy getting ready for kernel summit
> and then traveling too much.
>
No problem.

>
> On Fri, 18 Oct 2013 17:59:42 -0300
> "Geyslan G. Bem" <geyslan@xxxxxxxxx> wrote:
>
>> In 'system_tr_open()':
>> Fix possible 'dir' assignment after freeing it.
>>
>> In both functions:
>> Restructures logic conditions testing 'tracing_is_disabled()'
>> return before the others tests.
>> Centralizes the exiting in accordance to Coding Style, Chapter 7.
>>
>> Signed-off-by: Geyslan G. Bem <geyslan@xxxxxxxxx>
>> ---
>>  kernel/trace/trace_events.c | 46 +++++++++++++++++++++++----------------------
>>  1 file changed, 24 insertions(+), 22 deletions(-)
>>
>> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
>> index 368a4d5..0f56ebf 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -1060,7 +1060,10 @@ static int subsystem_open(struct inode *inode, struct file *filp)
>>       struct event_subsystem *system = NULL;
>>       struct ftrace_subsystem_dir *dir = NULL; /* Initialize for gcc */
>>       struct trace_array *tr;
>> -     int ret;
>> +     int ret = -ENODEV;
>> +
>> +     if (tracing_is_disabled())
>> +             return ret;
>>
>>       /* Make sure the system still exists */
>>       mutex_lock(&trace_types_lock);
>> @@ -1082,23 +1085,21 @@ static int subsystem_open(struct inode *inode, struct file *filp)
>>       mutex_unlock(&trace_types_lock);
>>
>>       if (!system)
>> -             return -ENODEV;
>> +             return ret;
>>
>>       /* Some versions of gcc think dir can be uninitialized here */
>>       WARN_ON(!dir);
>>
>>       /* Still need to increment the ref count of the system */
>> -     if (trace_array_get(tr) < 0) {
>> -             put_system(dir);
>> -             return -ENODEV;
>> -     }
>> +     ret = trace_array_get(tr);
>> +     if (ret)
>> +             goto err_get;
>>
>> -     ret = tracing_open_generic(inode, filp);
>> -     if (ret < 0) {
>> -             trace_array_put(tr);
>> -             put_system(dir);
>> -     }
>> +     filp->private_data = dir;
>> +     return 0;
>>
>> +err_get:
>> +     put_system(dir);
>>       return ret;
>
> I don't see any improvement in the above code, except for the initial
> check of tracing_is_disabled(). Just add:
>
>         if (tracing_is_disabled())
>                 return -ENODEV;
>
> no need for all the fancy work with using ret and goto.
>
>
>>  }
>>
>> @@ -1106,28 +1107,29 @@ static int system_tr_open(struct inode *inode, struct file *filp)
>>  {
>>       struct ftrace_subsystem_dir *dir;
>>       struct trace_array *tr = inode->i_private;
>> -     int ret;
>> +     int ret = -ENODEV;
>>
>> -     if (trace_array_get(tr) < 0)
>> -             return -ENODEV;
>> +     if (tracing_is_disabled())
>> +             return ret;
>> +
>> +     ret = trace_array_get(tr);
>> +     if (ret)
>> +             return ret;
>>
>>       /* 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;
>>
>
> Again, we don't need the goto and err_dir. The simple fix is to finish
> the function with:
>
>         ret = tracing_open_generic(inode, filp);
>         if (ret < 0) {
>                 trace_array_put(tr);
>                 kfree(dir);
>                 return ret;
>         }
>
>         filp->private_data = dir;
>
>         return 0;
>
> -- Steve
>
>> +err_dir:
>> +     trace_array_put(tr);
>>       return ret;
>>  }
>>
>

Done:
[PATCH v2] tracing: fix referencing after memory freeing and refactors code


-- 
Regards,

Geyslan G. Bem
hackingbits.com
--
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/