* [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter
@ 2014-07-11 19:06 Oleg Nesterov
2014-07-11 19:06 ` [PATCH 1/7] " Oleg Nesterov
` (7 more replies)
0 siblings, 8 replies; 21+ messages in thread
From: Oleg Nesterov @ 2014-07-11 19:06 UTC (permalink / raw)
To: Steven Rostedt
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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);
}
}
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/7] tracing: instance_rmdir() leaks ftrace_event_file->filter
2014-07-11 19:06 [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter Oleg Nesterov
@ 2014-07-11 19:06 ` Oleg Nesterov
2014-07-14 18:35 ` Steven Rostedt
2014-07-11 19:06 ` [PATCH 2/7] tracing: kill destroy_preds() and destroy_file_preds() Oleg Nesterov
` (6 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Oleg Nesterov @ 2014-07-11 19:06 UTC (permalink / raw)
To: Steven Rostedt
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/7] tracing: kill destroy_preds() and destroy_file_preds()
2014-07-11 19:06 [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter Oleg Nesterov
2014-07-11 19:06 ` [PATCH 1/7] " Oleg Nesterov
@ 2014-07-11 19:06 ` Oleg Nesterov
2014-07-11 19:06 ` [PATCH 3/7] tracing: kill destroy_call_preds() Oleg Nesterov
` (5 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Oleg Nesterov @ 2014-07-11 19:06 UTC (permalink / raw)
To: Steven Rostedt
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/7] tracing: kill destroy_call_preds()
2014-07-11 19:06 [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter Oleg Nesterov
2014-07-11 19:06 ` [PATCH 1/7] " Oleg Nesterov
2014-07-11 19:06 ` [PATCH 2/7] tracing: kill destroy_preds() and destroy_file_preds() Oleg Nesterov
@ 2014-07-11 19:06 ` Oleg Nesterov
2014-07-11 19:06 ` [PATCH 4/7] tracing: kill call_filter_disable() Oleg Nesterov
` (4 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Oleg Nesterov @ 2014-07-11 19:06 UTC (permalink / raw)
To: Steven Rostedt
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/7] tracing: kill call_filter_disable()
2014-07-11 19:06 [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter Oleg Nesterov
` (2 preceding siblings ...)
2014-07-11 19:06 ` [PATCH 3/7] tracing: kill destroy_call_preds() Oleg Nesterov
@ 2014-07-11 19:06 ` Oleg Nesterov
2014-07-14 19:03 ` Steven Rostedt
2014-07-11 19:06 ` [PATCH 5/7] tracing/uprobes: kill the dead TRACE_EVENT_FL_USE_CALL_FILTER logic Oleg Nesterov
` (3 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Oleg Nesterov @ 2014-07-11 19:06 UTC (permalink / raw)
To: Steven Rostedt
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 5/7] tracing/uprobes: kill the dead TRACE_EVENT_FL_USE_CALL_FILTER logic
2014-07-11 19:06 [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter Oleg Nesterov
` (3 preceding siblings ...)
2014-07-11 19:06 ` [PATCH 4/7] tracing: kill call_filter_disable() Oleg Nesterov
@ 2014-07-11 19:06 ` Oleg Nesterov
2014-07-14 19:04 ` Steven Rostedt
2014-07-15 0:18 ` Srikar Dronamraju
2014-07-11 19:06 ` [PATCH 6/7] tracing: kill ftrace_event_call->files Oleg Nesterov
` (2 subsequent siblings)
7 siblings, 2 replies; 21+ messages in thread
From: Oleg Nesterov @ 2014-07-11 19:06 UTC (permalink / raw)
To: Steven Rostedt
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 6/7] tracing: kill ftrace_event_call->files
2014-07-11 19:06 [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter Oleg Nesterov
` (4 preceding siblings ...)
2014-07-11 19:06 ` [PATCH 5/7] tracing/uprobes: kill the dead TRACE_EVENT_FL_USE_CALL_FILTER logic Oleg Nesterov
@ 2014-07-11 19:06 ` Oleg Nesterov
2014-07-14 19:06 ` Steven Rostedt
2014-07-11 19:06 ` [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name Oleg Nesterov
2014-07-14 13:54 ` [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter Namhyung Kim
7 siblings, 1 reply; 21+ messages in thread
From: Oleg Nesterov @ 2014-07-11 19:06 UTC (permalink / raw)
To: Steven Rostedt
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name
2014-07-11 19:06 [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter Oleg Nesterov
` (5 preceding siblings ...)
2014-07-11 19:06 ` [PATCH 6/7] tracing: kill ftrace_event_call->files Oleg Nesterov
@ 2014-07-11 19:06 ` Oleg Nesterov
2014-07-14 13:58 ` Namhyung Kim
2014-07-14 19:20 ` Steven Rostedt
2014-07-14 13:54 ` [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter Namhyung Kim
7 siblings, 2 replies; 21+ messages in thread
From: Oleg Nesterov @ 2014-07-11 19:06 UTC (permalink / raw)
To: Steven Rostedt
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter
2014-07-11 19:06 [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter Oleg Nesterov
` (6 preceding siblings ...)
2014-07-11 19:06 ` [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name Oleg Nesterov
@ 2014-07-14 13:54 ` Namhyung Kim
2014-07-14 16:15 ` Oleg Nesterov
7 siblings, 1 reply; 21+ messages in thread
From: Namhyung Kim @ 2014-07-14 13:54 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Steven Rostedt, Masami Hiramatsu, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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);
> }
> }
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name
2014-07-11 19:06 ` [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name Oleg Nesterov
@ 2014-07-14 13:58 ` Namhyung Kim
2014-07-14 16:02 ` Oleg Nesterov
2014-07-14 19:21 ` Steven Rostedt
2014-07-14 19:20 ` Steven Rostedt
1 sibling, 2 replies; 21+ messages in thread
From: Namhyung Kim @ 2014-07-14 13:58 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Steven Rostedt, Masami Hiramatsu, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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);
> }
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name
2014-07-14 13:58 ` Namhyung Kim
@ 2014-07-14 16:02 ` Oleg Nesterov
2014-07-14 19:21 ` Steven Rostedt
1 sibling, 0 replies; 21+ messages in thread
From: Oleg Nesterov @ 2014-07-14 16:02 UTC (permalink / raw)
To: Namhyung Kim
Cc: Steven Rostedt, Masami Hiramatsu, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter
2014-07-14 13:54 ` [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter Namhyung Kim
@ 2014-07-14 16:15 ` Oleg Nesterov
0 siblings, 0 replies; 21+ messages in thread
From: Oleg Nesterov @ 2014-07-14 16:15 UTC (permalink / raw)
To: Namhyung Kim
Cc: Steven Rostedt, Masami Hiramatsu, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/7] tracing: instance_rmdir() leaks ftrace_event_file->filter
2014-07-11 19:06 ` [PATCH 1/7] " Oleg Nesterov
@ 2014-07-14 18:35 ` Steven Rostedt
2014-07-14 19:03 ` Oleg Nesterov
0 siblings, 1 reply; 21+ messages in thread
From: Steven Rostedt @ 2014-07-14 18:35 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/7] tracing: instance_rmdir() leaks ftrace_event_file->filter
2014-07-14 18:35 ` Steven Rostedt
@ 2014-07-14 19:03 ` Oleg Nesterov
0 siblings, 0 replies; 21+ messages in thread
From: Oleg Nesterov @ 2014-07-14 19:03 UTC (permalink / raw)
To: Steven Rostedt
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/7] tracing: kill call_filter_disable()
2014-07-11 19:06 ` [PATCH 4/7] tracing: kill call_filter_disable() Oleg Nesterov
@ 2014-07-14 19:03 ` Steven Rostedt
0 siblings, 0 replies; 21+ messages in thread
From: Steven Rostedt @ 2014-07-14 19:03 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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;
> }
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/7] tracing/uprobes: kill the dead TRACE_EVENT_FL_USE_CALL_FILTER logic
2014-07-11 19:06 ` [PATCH 5/7] tracing/uprobes: kill the dead TRACE_EVENT_FL_USE_CALL_FILTER logic Oleg Nesterov
@ 2014-07-14 19:04 ` Steven Rostedt
2014-07-15 0:18 ` Srikar Dronamraju
1 sibling, 0 replies; 21+ messages in thread
From: Steven Rostedt @ 2014-07-14 19:04 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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);
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 6/7] tracing: kill ftrace_event_call->files
2014-07-11 19:06 ` [PATCH 6/7] tracing: kill ftrace_event_call->files Oleg Nesterov
@ 2014-07-14 19:06 ` Steven Rostedt
0 siblings, 0 replies; 21+ messages in thread
From: Steven Rostedt @ 2014-07-14 19:06 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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;
> /*
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name
2014-07-11 19:06 ` [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name Oleg Nesterov
2014-07-14 13:58 ` Namhyung Kim
@ 2014-07-14 19:20 ` Steven Rostedt
2014-07-14 19:23 ` Oleg Nesterov
1 sibling, 1 reply; 21+ messages in thread
From: Steven Rostedt @ 2014-07-14 19:20 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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);
> }
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name
2014-07-14 13:58 ` Namhyung Kim
2014-07-14 16:02 ` Oleg Nesterov
@ 2014-07-14 19:21 ` Steven Rostedt
1 sibling, 0 replies; 21+ messages in thread
From: Steven Rostedt @ 2014-07-14 19:21 UTC (permalink / raw)
To: Namhyung Kim
Cc: Oleg Nesterov, Masami Hiramatsu, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name
2014-07-14 19:20 ` Steven Rostedt
@ 2014-07-14 19:23 ` Oleg Nesterov
0 siblings, 0 replies; 21+ messages in thread
From: Oleg Nesterov @ 2014-07-14 19:23 UTC (permalink / raw)
To: Steven Rostedt
Cc: Masami Hiramatsu, Namhyung Kim, Srikar Dronamraju, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/7] tracing/uprobes: kill the dead TRACE_EVENT_FL_USE_CALL_FILTER logic
2014-07-11 19:06 ` [PATCH 5/7] tracing/uprobes: kill the dead TRACE_EVENT_FL_USE_CALL_FILTER logic Oleg Nesterov
2014-07-14 19:04 ` Steven Rostedt
@ 2014-07-15 0:18 ` Srikar Dronamraju
1 sibling, 0 replies; 21+ messages in thread
From: Srikar Dronamraju @ 2014-07-15 0:18 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Steven Rostedt, Masami Hiramatsu, Namhyung Kim, Tom Zanussi,
zhangwei(Jovi),
linux-kernel
* 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
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2014-07-15 0:18 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-11 19:06 [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter Oleg Nesterov
2014-07-11 19:06 ` [PATCH 1/7] " Oleg Nesterov
2014-07-14 18:35 ` Steven Rostedt
2014-07-14 19:03 ` Oleg Nesterov
2014-07-11 19:06 ` [PATCH 2/7] tracing: kill destroy_preds() and destroy_file_preds() Oleg Nesterov
2014-07-11 19:06 ` [PATCH 3/7] tracing: kill destroy_call_preds() Oleg Nesterov
2014-07-11 19:06 ` [PATCH 4/7] tracing: kill call_filter_disable() Oleg Nesterov
2014-07-14 19:03 ` Steven Rostedt
2014-07-11 19:06 ` [PATCH 5/7] tracing/uprobes: kill the dead TRACE_EVENT_FL_USE_CALL_FILTER logic Oleg Nesterov
2014-07-14 19:04 ` Steven Rostedt
2014-07-15 0:18 ` Srikar Dronamraju
2014-07-11 19:06 ` [PATCH 6/7] tracing: kill ftrace_event_call->files Oleg Nesterov
2014-07-14 19:06 ` Steven Rostedt
2014-07-11 19:06 ` [PATCH 7/7] tracing: change filter_free_subsystem_*() to check ->subsystem rather then ->name Oleg Nesterov
2014-07-14 13:58 ` Namhyung Kim
2014-07-14 16:02 ` Oleg Nesterov
2014-07-14 19:21 ` Steven Rostedt
2014-07-14 19:20 ` Steven Rostedt
2014-07-14 19:23 ` Oleg Nesterov
2014-07-14 13:54 ` [PATCH 0/7] tracing: instance_rmdir() leaks ftrace_event_file->filter Namhyung Kim
2014-07-14 16:15 ` Oleg Nesterov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.