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

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

[Geyslan G. Bem]

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@gmail.com>
---
 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;
 
 	/* 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;
 }
 
-- 
1.8.4

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

[Steven Rostedt]

On Thu, 17 Oct 2013 22:44:56 -0300
"Geyslan G. Bem" <geyslan@gmail.com> 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@gmail.com>
> ---
>  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.

-- 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;
>  }
>  

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

[Geyslan Gregório Bem]

2013/10/17 Steven Rostedt <rostedt@goodmis.org>:
> On Thu, 17 Oct 2013 22:44:56 -0300
> "Geyslan G. Bem" <geyslan@gmail.com> 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@gmail.com>
>> ---
>>  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:

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,

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;
>>  }
>>
>

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

[Geyslan Gregório Bem]

2013/10/18 Geyslan Gregório Bem <geyslan@gmail.com>:
> 2013/10/17 Steven Rostedt <rostedt@goodmis.org>:
>> On Thu, 17 Oct 2013 22:44:56 -0300
>> "Geyslan G. Bem" <geyslan@gmail.com> 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@gmail.com>
>>> ---
>>>  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;
>>>  }
>>>
>>

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

[Steven Rostedt]

On Fri, 18 Oct 2013 08:02:32 -0300
Geyslan Gregório Bem <geyslan@gmail.com> wrote:

> >>
> >> 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:

Right, tracing_is_disabled() should not be called as a fops method.

> >
> > 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);

Hmm, no, it needs the assignment too. But perhaps we could just open
code it. The tracing_open_generic() is more for fops then to be a
helper for functions that don't use it for their .open routine.

Thus, for subsystem_open() you could have at the start:

	if (tracing_is_disabled())
		return -ENODEV;


Then have:

-       ret = tracing_open_generic(inode, filp);
-       if (ret < 0) {
-               trace_array_put(tr);
-               put_system(dir);
-       }
+       filp->private_data = dir;


-- Steve


> 
> Regards,
> -- Geyslan

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

[Oleg Nesterov]

On 10/17, Steven Rostedt wrote:
>
> On Thu, 17 Oct 2013 22:44:56 -0300
> "Geyslan G. Bem" <geyslan@gmail.com> wrote:
>
> > and fix the possible 'dir'
> > assignment after freeing it.

This should be safe afaics, nobody will use it anyway. Even
subsystem_release() won't be called if .open() fails. But I agree
this doesn't look good.

> 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

The same for ftrace_enable_fops() and ftrace_event_filter_fops() at
least. The users of event_file_data() do not use ->private_data.

OTOH, say, trace_format_open() doesn't check tracing_disabled, so

> We could add a tracing_is_disabled() function to test instead.

perhaps it can have more callers.

Oleg.

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

[Oleg Nesterov]

Steven, et all, sorry for off-topic...

You probably know that kernel/trace/ is not trivial ;) and the fact
that cscope doesn't shows the callers in kernel/trace/trace_events.c
doesn't really help.

Fixed by the patch below, but I am not sure it is fine to uglify the
code to help the buggy tools. Is there any other way?

Oleg.
---

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 368a4d5..087fff1 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -68,6 +68,9 @@ static int system_refcount_dec(struct event_subsystem *system)
 		struct ftrace_event_file *___n;				\
 		list_for_each_entry_safe(file, ___n, &tr->events, list)
 
+/* Yes. Twice to not confuse csope */
+#define while_for_each_event_file()		\
+	}
 #define while_for_each_event_file()		\
 	}
 

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

[Geyslan Gregório Bem]

2013/10/19 Oleg Nesterov <oleg@redhat.com>:
> On 10/17, Steven Rostedt wrote:
>>
>> On Thu, 17 Oct 2013 22:44:56 -0300
>> "Geyslan G. Bem" <geyslan@gmail.com> wrote:
>>
>> > and fix the possible 'dir'
>> > assignment after freeing it.
>
> This should be safe afaics, nobody will use it anyway. Even
> subsystem_release() won't be called if .open() fails. But I agree
> this doesn't look good.

Right.
>
>> 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
>
> The same for ftrace_enable_fops() and ftrace_event_filter_fops() at
> least. The users of event_file_data() do not use ->private_data.
>

Aren't "ftrace_enable_fops" and "ftrace_event_filter_fops" structures?
About event_file_data() I think that the callers uses the
private_data. So, we have to analyze better.

> OTOH, say, trace_format_open() doesn't check tracing_disabled, so
>
>> We could add a tracing_is_disabled() function to test instead.
>
> perhaps it can have more callers.
>

static int trace_format_open(struct inode *inode, struct file *file)
{
    struct seq_file *m;
    int ret;

    ret = seq_open(file, &trace_format_seq_ops);
    if (ret < 0)
        return ret;

    m = file->private_data;
    m->private = file;

    return 0;
}

I really got confused here. The 'm' assignments are, to me, pointless.

> Oleg.
>

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

[Oleg Nesterov]

On 10/19, Geyslan Gregório Bem wrote:
>
> 2013/10/19 Oleg Nesterov <oleg@redhat.com>:
> > On 10/17, Steven Rostedt wrote:
> >>
> >> 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
> >
> > The same for ftrace_enable_fops() and ftrace_event_filter_fops() at
> > least. The users of event_file_data() do not use ->private_data.
> >
>
> Aren't "ftrace_enable_fops" and "ftrace_event_filter_fops" structures?

I meant, their ->open() methods.

> About event_file_data() I think that the callers uses the
> private_data. So, we have to analyze better.

No, event_file_data() uses ->i_private, filp->private_data is not used.
And it can't be used, it can point to the already destroyed/freed data.

but, as for seq_open() users,

> static int trace_format_open(struct inode *inode, struct file *file)
> {
>     struct seq_file *m;
>     int ret;
>
>     ret = seq_open(file, &trace_format_seq_ops);
>     if (ret < 0)
>         return ret;
>
>     m = file->private_data;
>     m->private = file;
>
>     return 0;
> }
>
> I really got confused here. The 'm' assignments are, to me, pointless.

I confused too... Why do you think it is pointless?

Just in case, not that after seq_open() ->private_data points to seq_file
but it is still "void *". And in this case ->private_data has nothing to
do with ->private_data  set by tracing_open_generic().

Oleg.

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

[Geyslan Gregório Bem]

2013/10/19 Oleg Nesterov <oleg@redhat.com>:
> On 10/19, Geyslan Gregório Bem wrote:
>>
>> 2013/10/19 Oleg Nesterov <oleg@redhat.com>:
>> > On 10/17, Steven Rostedt wrote:
>> >>
>> >> 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
>> >
>> > The same for ftrace_enable_fops() and ftrace_event_filter_fops() at
>> > least. The users of event_file_data() do not use ->private_data.
>> >
>>
>> Aren't "ftrace_enable_fops" and "ftrace_event_filter_fops" structures?
>
> I meant, their ->open() methods.

I see.
>
>> About event_file_data() I think that the callers uses the
>> private_data. So, we have to analyze better.
>
> No, event_file_data() uses ->i_private, filp->private_data is not used.
> And it can't be used, it can point to the already destroyed/freed data.

Ok. I got it.
>
> but, as for seq_open() users,
>
>> static int trace_format_open(struct inode *inode, struct file *file)
>> {
>>     struct seq_file *m;
>>     int ret;
>>
>>     ret = seq_open(file, &trace_format_seq_ops);
>>     if (ret < 0)
>>         return ret;
>>
>>     m = file->private_data;
>>     m->private = file;
>>
>>     return 0;
>> }
>>
>> I really got confused here. The 'm' assignments are, to me, pointless.
>
> I confused too... Why do you think it is pointless?
>
> Just in case, not that after seq_open() ->private_data points to seq_file
> but it is still "void *". And in this case ->private_data has nothing to
> do with ->private_data  set by tracing_open_generic().
>

My bad. I realized it now.
> Oleg.
>

Let's wait Steve's reply about further use of tracing_is_disabled().

Regards.
--Geyslan

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

[Steven Rostedt]

On Sat, 19 Oct 2013 11:41:10 -0300
Geyslan Gregório Bem <geyslan@gmail.com> wrote:
 
> Let's wait Steve's reply about further use of tracing_is_disabled().

Might want to ping me later. This weekend I'm taking my daughter to
colleges, and early Monday morning I'm leaving to Edinburgh (arriving
on Tuesday).

-- Steve

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

[Geyslan Gregório Bem]

2013/10/19 Steven Rostedt <rostedt@goodmis.org>:
> On Sat, 19 Oct 2013 11:41:10 -0300
> Geyslan Gregório Bem <geyslan@gmail.com> wrote:
>
>> Let's wait Steve's reply about further use of tracing_is_disabled().
>
> Might want to ping me later. This weekend I'm taking my daughter to
> colleges, and early Monday morning I'm leaving to Edinburgh (arriving
> on Tuesday).
>
> -- Steve
>

There is no problem, weekends are sacred to me. It's family time.

o/
--Geyslan