tracing: instance_rmdir() leaks ftrace_event_file->filter

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 <[email protected]>

---
 kernel/trace/trace_events.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

kernel/trace/trace_events.c +0 -0

index fdb2271..735608e 100644

Quoted 2 lines · On Fri, 11 Jul 2014 21:06:38 +0200 Oleg Nesterov <[email protected]> wrote:

(quoted diff) +0 -0

@@ -470,6 +470,7 @@ static void remove_event_file_dir(struct ftrace_event_file *file)
 

kernel/trace/trace_events.c static void remove_event_file_dir(struct ftrace_event_file *file) · line 476

Quoted 6 lines

kernel/trace/trace_events.c +1 -0

 	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.

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 <[email protected]>

---
 kernel/trace/trace_events_filter.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

kernel/trace/trace_events_filter.c +1 -6

index 1edec32..54a125c 100644
@@ -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;
 }

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 <[email protected]>

---
 kernel/trace/trace_uprobe.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

kernel/trace/trace_uprobe.c +1 -2

index 3c9b97e..33ff6a2 100644
@@ -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);

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 <[email protected]>

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 <[email protected]>

---
 include/linux/ftrace_event.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

include/linux/ftrace_event.h +0 -1

index f434d75..06c6faa 100644
@@ -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;
 	/*

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

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/trace/trace_events_filter.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

kernel/trace/trace_events_filter.c +2 -7

index 54a125c..59655bc 100644
@@ -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);
 	}

2014-07-11 (금), 21:06 +0200, Oleg Nesterov:

Quoted 2 lines

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.

2014-07-11 (금), 21:06 +0200, Oleg Nesterov:

Quoted 2 lines

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.

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

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/trace/trace_events_filter.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

kernel/trace/trace_events_filter.c +0 -0

index 54a125c..59655bc 100644

kernel/trace/trace_events_filter.c +1 -4

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

Quoted 6 lines · On Fri, 11 Jul 2014 21:06:55 +0200 Oleg Nesterov <[email protected]> wrote:

kernel/trace/trace_events_filter.c +0 -1

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

kernel/trace/trace_events_filter.c static void filter_free_subsystem_preds(struct event_subsystem *system, · line 851

Quoted 4 lines

kernel/trace/trace_events_filter.c +1 -2

 	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:

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.

kernel/trace/trace_events_filter.c +1 -12

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

Quoted 5 lines

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