[PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter

[PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter

[Oleg Nesterov]

Hello,

Sorry for delay, I was distracted by other problems.

To remind, we discussed the potential uprobes "mix perf/ftrace" changes and
found some off-topic problems. Lets start with ->filter fixes/cleanups.

So far I didn't even try to test this series, although it looks simple except
perhaps the last patch. I'll try to somehow test this tomorrow, but I do not
really now how.

So please review, these patches need acks anyway.

As for 1/7, I added BUG_ON(file->filter) into remove_event_file_dir() before
kmem_cache_free() to verify that yes, the leak does exist. Perhaps this patch
should go to stable, or at least to v3.16.

--------------------------------------------------------------------------------
And could someone explain me why apply_subsystem_event_filter("0") clears
->filter_string first, then the whole ->filter? It seems that the only
thing filter_free_subsystem_preds() should do is filter_disable(), no?
IOW, why the patch below (on top of this series) is wrong?

Oleg.

--- x/kernel/trace/trace_events_filter.c
+++ x/kernel/trace/trace_events_filter.c
@@ -831,17 +831,6 @@ static int __alloc_preds(struct event_filter *filter, int n_preds)
 	return 0;
 }
 
-static inline void __remove_filter(struct ftrace_event_file *file)
-{
-	struct ftrace_event_call *call = file->event_call;
-
-	filter_disable(file);
-	if (call->flags & TRACE_EVENT_FL_USE_CALL_FILTER)
-		remove_filter_string(call->filter);
-	else
-		remove_filter_string(file->filter);
-}
-
 static void filter_free_subsystem_preds(struct event_subsystem *system,
 					struct trace_array *tr)
 {
@@ -850,7 +839,7 @@ static void filter_free_subsystem_preds(struct event_subsystem *system,
 	list_for_each_entry(file, &tr->events, list) {
 		if (file->system->subsystem != system)
 			continue;
-		__remove_filter(file);
+		filter_disable(file);
 	}
 }
 

[PATCH 1/7] tracing: instance_rmdir() leaks ftrace_event_file->filter

[Oleg Nesterov]

instance_rmdir() path destroys the event files but forgets to free
file->filter. Change remove_event_file_dir() to free_event_filter().

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

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index fdb2271..735608e 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -470,6 +470,7 @@ static void remove_event_file_dir(struct ftrace_event_file *file)
 
 	list_del(&file->list);
 	remove_subsystem(file->system);
+	free_event_filter(file->filter);
 	kmem_cache_free(file_cachep, file);
 }
 
-- 
1.5.5.1

Re: [PATCH 1/7] tracing: instance_rmdir() leaks ftrace_event_file->filter

[Steven Rostedt]

On Fri, 11 Jul 2014 21:06:38 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> instance_rmdir() path destroys the event files but forgets to free
> file->filter. Change remove_event_file_dir() to free_event_filter().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/trace/trace_events.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index fdb2271..735608e 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -470,6 +470,7 @@ static void remove_event_file_dir(struct ftrace_event_file *file)
>  
>  	list_del(&file->list);
>  	remove_subsystem(file->system);
> +	free_event_filter(file->filter);
>  	kmem_cache_free(file_cachep, file);
>  }
>  

Thanks, looks like this goes back to 3.11. I'll start testing this and
get it out for stable as well.

-- Steve

Re: [PATCH 1/7] tracing: instance_rmdir() leaks ftrace_event_file->filter

[Oleg Nesterov]

On 07/14, Steven Rostedt wrote:
>
> On Fri, 11 Jul 2014 21:06:38 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > @@ -470,6 +470,7 @@ static void remove_event_file_dir(struct ftrace_event_file *file)
> >
> >  	list_del(&file->list);
> >  	remove_subsystem(file->system);
> > +	free_event_filter(file->filter);
> >  	kmem_cache_free(file_cachep, file);
> >  }
> >
>
> Thanks, looks like this goes back to 3.11. I'll start testing this and
> get it out for stable as well.

Thanks.

Just in case: sorry, I was distracted once again and didn't test 2-7 yet.
Although everything looks simple, even 7/7. But it needs v2 anyway, as
Namhyung pointed out this patch can also change replace_system_preds()
the same way.

Hopefully I'll do this tomorrow, but your review is needed anyway ;)

Oleg.

[PATCH 2/7] tracing: kill destroy_preds() and destroy_file_preds()

[Oleg Nesterov]

destroy_preds() makes no any sense.

The only caller, event_remove(), actually wants destroy_file_preds().
__trace_remove_event_call() does destroy_call_preds() which takes care
of call->filter.

And after the previous change we can simply remove destroy_preds() from
event_remove(), we are going to call remove_event_from_tracers() which
in turn calls remove_event_file_dir()->free_event_filter().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/ftrace_event.h       |    1 -
 kernel/trace/trace_events.c        |    1 -
 kernel/trace/trace_events_filter.c |   20 --------------------
 3 files changed, 0 insertions(+), 22 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index cff3106..738d465 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -404,7 +404,6 @@ enum event_trigger_type {
 	ETT_EVENT_ENABLE	= (1 << 3),
 };
 
-extern void destroy_preds(struct ftrace_event_file *file);
 extern void destroy_call_preds(struct ftrace_event_call *call);
 extern int filter_match_preds(struct event_filter *filter, void *rec);
 
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 735608e..d049a5e 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1621,7 +1621,6 @@ static void event_remove(struct ftrace_event_call *call)
 		if (file->event_call != call)
 			continue;
 		ftrace_event_enable_disable(file, 0);
-		destroy_preds(file);
 		/*
 		 * The do_for_each_event_file() is
 		 * a double loop. After finding the call for this
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 8a86319..30fc66f 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -810,26 +810,6 @@ void destroy_call_preds(struct ftrace_event_call *call)
 	call->filter = NULL;
 }
 
-static void destroy_file_preds(struct ftrace_event_file *file)
-{
-	__free_filter(file->filter);
-	file->filter = NULL;
-}
-
-/*
- * Called when destroying the ftrace_event_file.
- * The file is being freed, so we do not need to worry about
- * the file being currently used. This is for module code removing
- * the tracepoints from within it.
- */
-void destroy_preds(struct ftrace_event_file *file)
-{
-	if (file->event_call->flags & TRACE_EVENT_FL_USE_CALL_FILTER)
-		destroy_call_preds(file->event_call);
-	else
-		destroy_file_preds(file);
-}
-
 static struct event_filter *__alloc_filter(void)
 {
 	struct event_filter *filter;
-- 
1.5.5.1

[PATCH 3/7] tracing: kill destroy_call_preds()

[Oleg Nesterov]

Remove destroy_call_preds(). Its only caller, __trace_remove_event_call(),
can use free_event_filter() and nullify ->filter by hand.

Perhaps we could keep this trivial helper although imo it is pointless, but
then it should be static in trace_events.c.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/ftrace_event.h       |    1 -
 kernel/trace/trace_events.c        |    3 ++-
 kernel/trace/trace_events_filter.c |    6 ------
 3 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 738d465..f434d75 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -404,7 +404,6 @@ enum event_trigger_type {
 	ETT_EVENT_ENABLE	= (1 << 3),
 };
 
-extern void destroy_call_preds(struct ftrace_event_call *call);
 extern int filter_match_preds(struct event_filter *filter, void *rec);
 
 extern int filter_check_discard(struct ftrace_event_file *file, void *rec,
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index d049a5e..8690a3f 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1748,7 +1748,8 @@ static void __trace_remove_event_call(struct ftrace_event_call *call)
 {
 	event_remove(call);
 	trace_destroy_fields(call);
-	destroy_call_preds(call);
+	free_event_filter(call->filter);
+	call->filter = NULL;
 }
 
 static int probe_remove_event_call(struct ftrace_event_call *call)
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 30fc66f..1edec32 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -804,12 +804,6 @@ void free_event_filter(struct event_filter *filter)
 	__free_filter(filter);
 }
 
-void destroy_call_preds(struct ftrace_event_call *call)
-{
-	__free_filter(call->filter);
-	call->filter = NULL;
-}
-
 static struct event_filter *__alloc_filter(void)
 {
 	struct event_filter *filter;
-- 
1.5.5.1

[PATCH 4/7] tracing: kill call_filter_disable()

[Oleg Nesterov]

It seems that the only purpose of call_filter_disable() is to
make filter_disable() less clear and symmetrical, remove it.

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

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 1edec32..54a125c 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -774,17 +774,12 @@ static void __free_preds(struct event_filter *filter)
 	filter->n_preds = 0;
 }
 
-static void call_filter_disable(struct ftrace_event_call *call)
-{
-	call->flags &= ~TRACE_EVENT_FL_FILTERED;
-}
-
 static void filter_disable(struct ftrace_event_file *file)
 {
 	struct ftrace_event_call *call = file->event_call;
 
 	if (call->flags & TRACE_EVENT_FL_USE_CALL_FILTER)
-		call_filter_disable(call);
+		call->flags &= ~TRACE_EVENT_FL_FILTERED;
 	else
 		file->flags &= ~FTRACE_EVENT_FL_FILTERED;
 }
-- 
1.5.5.1

Re: [PATCH 4/7] tracing: kill call_filter_disable()

[Steven Rostedt]

On Fri, 11 Jul 2014 21:06:47 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> It seems that the only purpose of call_filter_disable() is to
> make filter_disable() less clear and symmetrical, remove it.

But what will replace the purpose of making it less symmetrical and
clear?

This is the result of modifications over time.

-- Steve

> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/trace/trace_events_filter.c |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 1edec32..54a125c 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -774,17 +774,12 @@ static void __free_preds(struct event_filter *filter)
>  	filter->n_preds = 0;
>  }
>  
> -static void call_filter_disable(struct ftrace_event_call *call)
> -{
> -	call->flags &= ~TRACE_EVENT_FL_FILTERED;
> -}
> -
>  static void filter_disable(struct ftrace_event_file *file)
>  {
>  	struct ftrace_event_call *call = file->event_call;
>  
>  	if (call->flags & TRACE_EVENT_FL_USE_CALL_FILTER)
> -		call_filter_disable(call);
> +		call->flags &= ~TRACE_EVENT_FL_FILTERED;
>  	else
>  		file->flags &= ~FTRACE_EVENT_FL_FILTERED;
>  }

[PATCH 5/7] tracing/uprobes: kill the dead TRACE_EVENT_FL_USE_CALL_FILTER logic

[Oleg Nesterov]

alloc_trace_uprobe() sets TRACE_EVENT_FL_USE_CALL_FILTER for unknown
reason and this is simply wrong. Fortunately this has no effect because
register_uprobe_event() clears call->flags after that.

Kill both. This trace_uprobe was kzalloc'ed and we realy on this fact
anyway.

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

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 3c9b97e..33ff6a2 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -265,7 +265,6 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
 	if (is_ret)
 		tu->consumer.ret_handler = uretprobe_dispatcher;
 	init_trace_uprobe_filter(&tu->filter);
-	tu->tp.call.flags |= TRACE_EVENT_FL_USE_CALL_FILTER;
 	return tu;
 
 error:
@@ -1292,7 +1291,7 @@ static int register_uprobe_event(struct trace_uprobe *tu)
 		kfree(call->print_fmt);
 		return -ENODEV;
 	}
-	call->flags = 0;
+
 	call->class->reg = trace_uprobe_register;
 	call->data = tu;
 	ret = trace_add_event_call(call);
-- 
1.5.5.1

Re: [PATCH 5/7] tracing/uprobes: kill the dead TRACE_EVENT_FL_USE_CALL_FILTER logic

[Steven Rostedt]

On Fri, 11 Jul 2014 21:06:50 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> alloc_trace_uprobe() sets TRACE_EVENT_FL_USE_CALL_FILTER for unknown
> reason and this is simply wrong. 

The reason is not unknown. The reason is "cut and paste".

-- Steve

> Fortunately this has no effect because
> register_uprobe_event() clears call->flags after that.
> 
> Kill both. This trace_uprobe was kzalloc'ed and we realy on this fact
> anyway.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/trace/trace_uprobe.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 3c9b97e..33ff6a2 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -265,7 +265,6 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
>  	if (is_ret)
>  		tu->consumer.ret_handler = uretprobe_dispatcher;
>  	init_trace_uprobe_filter(&tu->filter);
> -	tu->tp.call.flags |= TRACE_EVENT_FL_USE_CALL_FILTER;
>  	return tu;
>  
>  error:
> @@ -1292,7 +1291,7 @@ static int register_uprobe_event(struct trace_uprobe *tu)
>  		kfree(call->print_fmt);
>  		return -ENODEV;
>  	}
> -	call->flags = 0;
> +
>  	call->class->reg = trace_uprobe_register;
>  	call->data = tu;
>  	ret = trace_add_event_call(call);

Re: [PATCH 5/7] tracing/uprobes: kill the dead TRACE_EVENT_FL_USE_CALL_FILTER logic

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2014-07-11 21:06:50]:

> alloc_trace_uprobe() sets TRACE_EVENT_FL_USE_CALL_FILTER for unknown
> reason and this is simply wrong. Fortunately this has no effect because
> register_uprobe_event() clears call->flags after that.
> 
> Kill both. This trace_uprobe was kzalloc'ed and we realy on this fact
> anyway.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

-- 
Thanks and Regards
Srikar Dronamraju

[PATCH 6/7] tracing: kill ftrace_event_call->files

[Oleg Nesterov]

Remove ftrace_event_call->files. It has no users, and in fact even
the commit ae63b31e4d0e "tracing: Separate out trace events from
global variables" which added this member did not use it.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/ftrace_event.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index f434d75..06c6faa 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -272,7 +272,6 @@ struct ftrace_event_call {
 	struct trace_event	event;
 	const char		*print_fmt;
 	struct event_filter	*filter;
-	struct list_head	*files;
 	void			*mod;
 	void			*data;
 	/*
-- 
1.5.5.1

Re: [PATCH 6/7] tracing: kill ftrace_event_call->files

[Steven Rostedt]

On Fri, 11 Jul 2014 21:06:53 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> Remove ftrace_event_call->files. It has no users, and in fact even
> the commit ae63b31e4d0e "tracing: Separate out trace events from
> global variables" which added this member did not use it.
> 

Ouch!

OK, I swear I used it before I committed the change. I just stopped
using it before I committed the change as well.

-- Steve

> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  include/linux/ftrace_event.h |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index f434d75..06c6faa 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -272,7 +272,6 @@ struct ftrace_event_call {
>  	struct trace_event	event;
>  	const char		*print_fmt;
>  	struct event_filter	*filter;
> -	struct list_head	*files;
>  	void			*mod;
>  	void			*data;
>  	/*

[PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name

[Oleg Nesterov]

filter_free_subsystem_preds() and filter_free_subsystem_filters()
can rely on file->system->subsystem and avoid strcmp().

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

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 54a125c..59655bc 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -846,13 +846,10 @@ static void filter_free_subsystem_preds(struct event_subsystem *system,
 					struct trace_array *tr)
 {
 	struct ftrace_event_file *file;
-	struct ftrace_event_call *call;
 
 	list_for_each_entry(file, &tr->events, list) {
-		call = file->event_call;
-		if (strcmp(call->class->system, system->name) != 0)
+		if (file->system->subsystem != system)
 			continue;
-
 		__remove_filter(file);
 	}
 }
@@ -874,11 +871,9 @@ static void filter_free_subsystem_filters(struct event_subsystem *system,
 					  struct trace_array *tr)
 {
 	struct ftrace_event_file *file;
-	struct ftrace_event_call *call;
 
 	list_for_each_entry(file, &tr->events, list) {
-		call = file->event_call;
-		if (strcmp(call->class->system, system->name) != 0)
+		if (file->system->subsystem != system)
 			continue;
 		__free_subsystem_filter(file);
 	}
-- 
1.5.5.1

Re: [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name

[Namhyung Kim]

2014-07-11 (금), 21:06 +0200, Oleg Nesterov:
> filter_free_subsystem_preds() and filter_free_subsystem_filters()
> can rely on file->system->subsystem and avoid strcmp().

I guess the same can go to the replace_system_preds() too.

Thanks,
Namhyung


> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/trace/trace_events_filter.c |    9 ++-------
>  1 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 54a125c..59655bc 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -846,13 +846,10 @@ static void filter_free_subsystem_preds(struct event_subsystem *system,
>  					struct trace_array *tr)
>  {
>  	struct ftrace_event_file *file;
> -	struct ftrace_event_call *call;
>  
>  	list_for_each_entry(file, &tr->events, list) {
> -		call = file->event_call;
> -		if (strcmp(call->class->system, system->name) != 0)
> +		if (file->system->subsystem != system)
>  			continue;
> -
>  		__remove_filter(file);
>  	}
>  }
> @@ -874,11 +871,9 @@ static void filter_free_subsystem_filters(struct event_subsystem *system,
>  					  struct trace_array *tr)
>  {
>  	struct ftrace_event_file *file;
> -	struct ftrace_event_call *call;
>  
>  	list_for_each_entry(file, &tr->events, list) {
> -		call = file->event_call;
> -		if (strcmp(call->class->system, system->name) != 0)
> +		if (file->system->subsystem != system)
>  			continue;
>  		__free_subsystem_filter(file);
>  	}

Re: [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name

[Oleg Nesterov]

On 07/14, Namhyung Kim wrote:
>
> 2014-07-11 (금), 21:06 +0200, Oleg Nesterov:
> > filter_free_subsystem_preds() and filter_free_subsystem_filters()
> > can rely on file->system->subsystem and avoid strcmp().
>
> I guess the same can go to the replace_system_preds() too.

Hmm, indeed ;) I'll send "v2 7/7".

Thanks!

Oleg.

Re: [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name

[Steven Rostedt]

On Mon, 14 Jul 2014 22:58:19 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> 2014-07-11 (금), 21:06 +0200, Oleg Nesterov:
> > filter_free_subsystem_preds() and filter_free_subsystem_filters()
> > can rely on file->system->subsystem and avoid strcmp().
> 
> I guess the same can go to the replace_system_preds() too.
> 

And it looks like you can pass dir there instead of system as well.

-- Steve

Re: [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name

[Steven Rostedt]

On Fri, 11 Jul 2014 21:06:55 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> filter_free_subsystem_preds() and filter_free_subsystem_filters()
> can rely on file->system->subsystem and avoid strcmp().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/trace/trace_events_filter.c |    9 ++-------
>  1 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 54a125c..59655bc 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -846,13 +846,10 @@ static void filter_free_subsystem_preds(struct event_subsystem *system,
>  					struct trace_array *tr)
>  {
>  	struct ftrace_event_file *file;
> -	struct ftrace_event_call *call;
>  
>  	list_for_each_entry(file, &tr->events, list) {
> -		call = file->event_call;
> -		if (strcmp(call->class->system, system->name) != 0)
> +		if (file->system->subsystem != system)

This is only used on one place. What about just passing in dir instead
of system and then do:

		if (file->system != dir)
>  			continue;

-- Steve

> -
>  		__remove_filter(file);
>  	}
>  }
> @@ -874,11 +871,9 @@ static void filter_free_subsystem_filters(struct event_subsystem *system,
>  					  struct trace_array *tr)
>  {
>  	struct ftrace_event_file *file;
> -	struct ftrace_event_call *call;
>  
>  	list_for_each_entry(file, &tr->events, list) {
> -		call = file->event_call;
> -		if (strcmp(call->class->system, system->name) != 0)
> +		if (file->system->subsystem != system)
>  			continue;
>  		__free_subsystem_filter(file);
>  	}

Re: [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name

[Oleg Nesterov]

On 07/14, Steven Rostedt wrote:
>
> On Fri, 11 Jul 2014 21:06:55 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
> 
> > @@ -846,13 +846,10 @@ static void filter_free_subsystem_preds(struct event_subsystem *system,
> >  					struct trace_array *tr)
> >  {
> >  	struct ftrace_event_file *file;
> > -	struct ftrace_event_call *call;
> >  
> >  	list_for_each_entry(file, &tr->events, list) {
> > -		call = file->event_call;
> > -		if (strcmp(call->class->system, system->name) != 0)
> > +		if (file->system->subsystem != system)
> 
> This is only used on one place. What about just passing in dir instead
> of system and then do:

I swear, I too thought about this ;) OK, in v2.

Oleg.

Re: [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter

[Namhyung Kim]

Hi Oleg,

2014-07-11 (금), 21:06 +0200, Oleg Nesterov:
> Hello,
> 
> Sorry for delay, I was distracted by other problems.
> 
> To remind, we discussed the potential uprobes "mix perf/ftrace" changes and
> found some off-topic problems. Lets start with ->filter fixes/cleanups.
> 
> So far I didn't even try to test this series, although it looks simple except
> perhaps the last patch. I'll try to somehow test this tomorrow, but I do not
> really now how.
> 
> So please review, these patches need acks anyway.
> 
> As for 1/7, I added BUG_ON(file->filter) into remove_event_file_dir() before
> kmem_cache_free() to verify that yes, the leak does exist. Perhaps this patch
> should go to stable, or at least to v3.16.
> 
> --------------------------------------------------------------------------------
> And could someone explain me why apply_subsystem_event_filter("0") clears
> ->filter_string first, then the whole ->filter? It seems that the only
> thing filter_free_subsystem_preds() should do is filter_disable(), no?
> IOW, why the patch below (on top of this series) is wrong?

I also think that the original code is bit strange.  I agree with your
change and name of the function should be changed to something like
'filter_disable_subsystem_filters' IMHO (it does nothing with preds).
With this change, the apply_subsystem_event_filter can simply do below:

	if (!strcmp(strstrip(filter_string), "0")) {
		filter_disable_subsystem_filters(system, tr);
		/* Ensure all filters are no longer used */
		synchronize_sched();
		filter_free_subsystem_filters(system, tr);
		__free_filter(system->filter);
		system->filter = NULL;
		goto out_unlock;
	}

Thanks,
Namhyung


> 
> Oleg.
> 
> --- x/kernel/trace/trace_events_filter.c
> +++ x/kernel/trace/trace_events_filter.c
> @@ -831,17 +831,6 @@ static int __alloc_preds(struct event_filter *filter, int n_preds)
>  	return 0;
>  }
>  
> -static inline void __remove_filter(struct ftrace_event_file *file)
> -{
> -	struct ftrace_event_call *call = file->event_call;
> -
> -	filter_disable(file);
> -	if (call->flags & TRACE_EVENT_FL_USE_CALL_FILTER)
> -		remove_filter_string(call->filter);
> -	else
> -		remove_filter_string(file->filter);
> -}
> -
>  static void filter_free_subsystem_preds(struct event_subsystem *system,
>  					struct trace_array *tr)
>  {
> @@ -850,7 +839,7 @@ static void filter_free_subsystem_preds(struct event_subsystem *system,
>  	list_for_each_entry(file, &tr->events, list) {
>  		if (file->system->subsystem != system)
>  			continue;
> -		__remove_filter(file);
> +		filter_disable(file);
>  	}
>  }
>  
> 

Re: [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter

[Oleg Nesterov]

On 07/14, Namhyung Kim wrote:
>
> > And could someone explain me why apply_subsystem_event_filter("0") clears
> > ->filter_string first, then the whole ->filter? It seems that the only
> > thing filter_free_subsystem_preds() should do is filter_disable(), no?
> > IOW, why the patch below (on top of this series) is wrong?
>
> I also think that the original code is bit strange.  I agree with your
> change and name of the function should be changed to something like
> 'filter_disable_subsystem_filters' IMHO (it does nothing with preds).
> With this change, the apply_subsystem_event_filter can simply do below:
>
> 	if (!strcmp(strstrip(filter_string), "0")) {
> 		filter_disable_subsystem_filters(system, tr);
> 		/* Ensure all filters are no longer used */
> 		synchronize_sched();
> 		filter_free_subsystem_filters(system, tr);
> 		__free_filter(system->filter);
> 		system->filter = NULL;
> 		goto out_unlock;
> 	}

Yes, thanks, this was my point.

And I thought that I saw the same pattern somewhere else, but can't recall
where... Will try to recheck.

Oleg.