[PATCH 0/1] tracing: Simplify trace_array_get()

[PATCH 0/1] tracing: Simplify trace_array_get()

[Oleg Nesterov]

Hello,

I am a bit puzzled by trace_array_get... Could you please at
least review this change?

It is very minor and it is not needed for the changes we are
discussing, just I am worried I could miss something subtle.

IOW, I won't argue if you prefer the current "explicit" on-list
check, but I will appreciate if you explain why this change can
be wrong.

Oleg.

[PATCH 1/1] tracing: Simplify trace_array_get()

[Oleg Nesterov]

trace_array_get() scans the global ftrace_trace_arrays list
to ensure that "this_tr" was not removed by instance_delete().

This looks a bit confusing, we can simply use list_empty() with
the same result. list_empty() == F can not be false positive,
new_instance_create() does everything under the same lock and
tracer_alloc_buffers() which plays with global_trace is __init.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/trace/trace.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 0cd500b..ee471fb 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -206,16 +206,12 @@ LIST_HEAD(ftrace_trace_arrays);
 
 int trace_array_get(struct trace_array *this_tr)
 {
-	struct trace_array *tr;
 	int ret = -ENODEV;
 
 	mutex_lock(&trace_types_lock);
-	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
-		if (tr == this_tr) {
-			tr->ref++;
-			ret = 0;
-			break;
-		}
+	if (!list_empty(&this_tr->list)) {
+		this_tr->ref++;
+		ret = 0;
 	}
 	mutex_unlock(&trace_types_lock);
 
@@ -6019,7 +6015,7 @@ static int instance_delete(const char *name)
 	if (tr->ref)
 		goto out_unlock;
 
-	list_del(&tr->list);
+	list_del_init(&tr->list); /* trace_array_get() checks list_empty() */
 
 	event_trace_del_tracer(tr);
 	debugfs_remove_recursive(tr->dir);
-- 
1.5.5.1

Re: [PATCH 1/1] tracing: Simplify trace_array_get()

[Steven Rostedt]

On Fri, 2013-07-19 at 17:51 +0200, Oleg Nesterov wrote:
> trace_array_get() scans the global ftrace_trace_arrays list
> to ensure that "this_tr" was not removed by instance_delete().
> 
> This looks a bit confusing, we can simply use list_empty() with
> the same result. list_empty() == F can not be false positive,
> new_instance_create() does everything under the same lock and
> tracer_alloc_buffers() which plays with global_trace is __init.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/trace/trace.c |   12 ++++--------
>  1 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 0cd500b..ee471fb 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -206,16 +206,12 @@ LIST_HEAD(ftrace_trace_arrays);
>  
>  int trace_array_get(struct trace_array *this_tr)
>  {
> -	struct trace_array *tr;
>  	int ret = -ENODEV;
>  
>  	mutex_lock(&trace_types_lock);
> -	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
> -		if (tr == this_tr) {
> -			tr->ref++;
> -			ret = 0;
> -			break;
> -		}
> +	if (!list_empty(&this_tr->list)) {

Because this_tr can be freed outside the lock. Accessing this_tr->list
can cause a crash.

The point of the search is, if it's on the list, it will not be freed.

-- Steve


> +		this_tr->ref++;
> +		ret = 0;
>  	}
>  	mutex_unlock(&trace_types_lock);
>  
> @@ -6019,7 +6015,7 @@ static int instance_delete(const char *name)
>  	if (tr->ref)
>  		goto out_unlock;
>  
> -	list_del(&tr->list);
> +	list_del_init(&tr->list); /* trace_array_get() checks list_empty() */
>  
>  	event_trace_del_tracer(tr);
>  	debugfs_remove_recursive(tr->dir);

Re: [PATCH 1/1] tracing: Simplify trace_array_get()

[Oleg Nesterov]

On 07/19, Steven Rostedt wrote:
>
> On Fri, 2013-07-19 at 17:51 +0200, Oleg Nesterov wrote:
> >  	mutex_lock(&trace_types_lock);
> > -	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
> > -		if (tr == this_tr) {
> > -			tr->ref++;
> > -			ret = 0;
> > -			break;
> > -		}
> > +	if (!list_empty(&this_tr->list)) {
>
> Because this_tr can be freed outside the lock. Accessing this_tr->list
> can cause a crash.

Aaah, indeed.

Thanks Steven!

Oleg.

Re: [PATCH 1/1] tracing: Simplify trace_array_get()

[Oleg Nesterov]

On 07/19, Oleg Nesterov wrote:
>
> On 07/19, Steven Rostedt wrote:
> >
> > On Fri, 2013-07-19 at 17:51 +0200, Oleg Nesterov wrote:
> > >  	mutex_lock(&trace_types_lock);
> > > -	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
> > > -		if (tr == this_tr) {
> > > -			tr->ref++;
> > > -			ret = 0;
> > > -			break;
> > > -		}
> > > +	if (!list_empty(&this_tr->list)) {
> >
> > Because this_tr can be freed outside the lock. Accessing this_tr->list
> > can cause a crash.
>
> Aaah, indeed.
>
> Thanks Steven!

Yes. But unless I missed something again this logic doesn't look exactly
correct. Because it seems that trace_array_get() can succeed when it
shoudn't.

trace_array_get() can race with instance_delete() + new_instance_create(),
and _create()->kzalloc() can return the same memory which was freed by
_delete().

No?

Oleg.

Re: [PATCH 1/1] tracing: Simplify trace_array_get()

[Steven Rostedt]

On Fri, 2013-07-19 at 19:20 +0200, Oleg Nesterov wrote:

> Yes. But unless I missed something again this logic doesn't look exactly
> correct. Because it seems that trace_array_get() can succeed when it
> shoudn't.
> 
> trace_array_get() can race with instance_delete() + new_instance_create(),
> and _create()->kzalloc() can return the same memory which was freed by
> _delete().
> 
> No?

Correct, but I don't see that as a major problem, do you?

What would happen in that case, is that an event might be enabled or
disabled in another buffer instance. As that can only happen by the root
user, it would be the root user doing multiple things at the same time
to cause it. They might get a strange result at worse, but that would
also mean they were trying to add and delete instances while reading
those same instances. Bad root, bad!

-- Steve

Re: [PATCH 1/1] tracing: Simplify trace_array_get()

[Steven Rostedt]

On Fri, 2013-07-19 at 13:35 -0400, Steven Rostedt wrote:

> What would happen in that case, is that an event might be enabled or
> disabled in another buffer instance. As that can only happen by the root
> user, it would be the root user doing multiple things at the same time
> to cause it. They might get a strange result at worse, but that would
> also mean they were trying to add and delete instances while reading
> those same instances. Bad root, bad!

In fact, I even mentioned this race in the commit log.

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/kernel/trace/trace.c?id=ff451961a8b2a17667a7bfa39c86fb9b351445db

-- Steve

Re: [PATCH 1/1] tracing: Simplify trace_array_get()

[Oleg Nesterov]

On 07/19, Steven Rostedt wrote:
>
> On Fri, 2013-07-19 at 19:20 +0200, Oleg Nesterov wrote:
>
> > Yes. But unless I missed something again this logic doesn't look exactly
> > correct. Because it seems that trace_array_get() can succeed when it
> > shoudn't.
> >
> > trace_array_get() can race with instance_delete() + new_instance_create(),
> > and _create()->kzalloc() can return the same memory which was freed by
> > _delete().
> >
> > No?
>
> Correct, but I don't see that as a major problem, do you?

Neither me. I should have mentioned this.

Except "might get a strange result" as you said.

But I have to admit, I am still trying to find something really bad ;)

Oleg.

Re: [PATCH 1/1] tracing: Simplify trace_array_get()

[Oleg Nesterov]

On 07/19, Oleg Nesterov wrote:
>
> On 07/19, Steven Rostedt wrote:
> >
> > Correct, but I don't see that as a major problem, do you?
>
> Neither me. I should have mentioned this.
>
> Except "might get a strange result" as you said.
>
> But I have to admit, I am still trying to find something really bad ;)

And it seems I have found... I'll try to report tomorrow.

Steven, Masami. I do remember I promised to finish/resend the RFC
patches on Friday.

But I decided to take a break and try to see what else we should
fix, and it seems that we have the bugs we did not discuss yet.
And _I hope_ the fixes should not be "controversial", so I'll try
to send them first.

Oleg.