* [PATCH 1/4] tracing/events: don't say hi when loading the trace event sample
@ 2009-05-06 2:32 Li Zefan
2009-05-06 2:32 ` [PATCH 2/4] tracing/events: make SAMPLE_TRACE_EVENTS default to n Li Zefan
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Li Zefan @ 2009-05-06 2:32 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Frederic Weisbecker, Ingo Molnar, LKML
The sample is useful for testing, and I'm using it. But after
loading the module, it keeps saying hi every 10 seconds, this may
be disturbing.
Also Steven said commenting out the "hi" helped in causing races. :)
[ Impact: make testing a bit easier ]
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
samples/trace_events/trace-events-sample.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/samples/trace_events/trace-events-sample.c b/samples/trace_events/trace-events-sample.c
index f33b3ba..aabc4e9 100644
--- a/samples/trace_events/trace-events-sample.c
+++ b/samples/trace_events/trace-events-sample.c
@@ -16,10 +16,6 @@ static void simple_thread_func(int cnt)
set_current_state(TASK_INTERRUPTIBLE);
schedule_timeout(HZ);
trace_foo_bar("hello", cnt);
-
- if (!(cnt % 10))
- /* It is really important that I say "hi!" */
- printk(KERN_EMERG "hi!\n");
}
static int simple_thread(void *arg)
--
1.5.4.rc3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/4] tracing/events: make SAMPLE_TRACE_EVENTS default to n
2009-05-06 2:32 [PATCH 1/4] tracing/events: don't say hi when loading the trace event sample Li Zefan
@ 2009-05-06 2:32 ` Li Zefan
2009-05-06 12:19 ` [tip:tracing/core] " tip-bot for Li Zefan
2009-05-06 2:33 ` [PATCH 3/4] tracing/events: fix memory leak when unloading module Li Zefan
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Li Zefan @ 2009-05-06 2:32 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Frederic Weisbecker, Ingo Molnar, LKML
Normally a config should be default to n. This patch also makes the
sample module-only, like SAMPLE_MARKERS and SAMPLE_TRACEPOINTS.
[ Impact: don't build trace event sample by default ]
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
samples/Kconfig | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/samples/Kconfig b/samples/Kconfig
index 93f41c0..b75d28c 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -20,9 +20,8 @@ config SAMPLE_TRACEPOINTS
This build tracepoints example modules.
config SAMPLE_TRACE_EVENTS
- tristate "Build trace_events examples"
- depends on EVENT_TRACING
- default m
+ tristate "Build trace_events examples -- loadable modules only"
+ depends on EVENT_TRACING && m
help
This build trace event example modules.
--
1.5.4.rc3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/4] tracing/events: fix memory leak when unloading module
2009-05-06 2:32 [PATCH 1/4] tracing/events: don't say hi when loading the trace event sample Li Zefan
2009-05-06 2:32 ` [PATCH 2/4] tracing/events: make SAMPLE_TRACE_EVENTS default to n Li Zefan
@ 2009-05-06 2:33 ` Li Zefan
2009-05-06 2:43 ` Steven Rostedt
2009-05-06 12:19 ` [tip:tracing/core] " tip-bot for Li Zefan
2009-05-06 2:33 ` [PATCH 4/4] tracing/events: fix concurrent access to ftrace_events list Li Zefan
` (2 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: Li Zefan @ 2009-05-06 2:33 UTC (permalink / raw)
To: Li Zefan
Cc: Steven Rostedt, Frederic Weisbecker, Ingo Molnar, LKML, Tom Zanussi
When unloading a module, memory allocated by init_preds() and
trace_define_field() is not freed.
[ Impact: fix memory leak ]
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
include/linux/ftrace_event.h | 1 +
kernel/trace/trace_events.c | 18 ++++++++++++++++++
kernel/trace/trace_events_filter.c | 22 +++++++++++++++-------
3 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 5fff40c..662c1be 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -116,6 +116,7 @@ struct ftrace_event_call {
#define MAX_FILTER_STR_VAL 128
extern int init_preds(struct ftrace_event_call *call);
+extern void destroy_preds(struct ftrace_event_call *call);
extern int filter_match_preds(struct ftrace_event_call *call, void *rec);
extern int filter_current_check_discard(struct ftrace_event_call *call,
void *rec,
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 9998cc8..72d619d 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -60,6 +60,22 @@ err:
}
EXPORT_SYMBOL_GPL(trace_define_field);
+#ifdef CONFIG_MODULES
+
+static void trace_destroy_fields(struct ftrace_event_call *call)
+{
+ struct ftrace_event_field *field, *next;
+
+ list_for_each_entry_safe(field, next, &call->fields, link) {
+ list_del(&field->link);
+ kfree(field->type);
+ kfree(field->name);
+ kfree(field);
+ }
+}
+
+#endif /* CONFIG_MODULES */
+
static void ftrace_clear_events(void)
{
struct ftrace_event_call *call;
@@ -925,6 +941,8 @@ static void trace_module_remove_events(struct module *mod)
unregister_ftrace_event(call->event);
debugfs_remove_recursive(call->dir);
list_del(&call->list);
+ trace_destroy_fields(call);
+ destroy_preds(call);
}
}
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index ac30de3..97bd140 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -371,6 +371,20 @@ static void filter_disable_preds(struct ftrace_event_call *call)
filter->preds[i]->fn = filter_pred_none;
}
+void destroy_preds(struct ftrace_event_call *call)
+{
+ struct event_filter *filter = call->filter;
+ int i;
+
+ for (i = 0; i < MAX_FILTER_PRED; i++) {
+ if (filter->preds[i])
+ filter_free_pred(filter->preds[i]);
+ }
+ kfree(filter->preds);
+ kfree(filter);
+ call->filter = NULL;
+}
+
int init_preds(struct ftrace_event_call *call)
{
struct event_filter *filter;
@@ -399,13 +413,7 @@ int init_preds(struct ftrace_event_call *call)
return 0;
oom:
- for (i = 0; i < MAX_FILTER_PRED; i++) {
- if (filter->preds[i])
- filter_free_pred(filter->preds[i]);
- }
- kfree(filter->preds);
- kfree(call->filter);
- call->filter = NULL;
+ destroy_preds(call);
return -ENOMEM;
}
--
1.5.4.rc3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 4/4] tracing/events: fix concurrent access to ftrace_events list
2009-05-06 2:32 [PATCH 1/4] tracing/events: don't say hi when loading the trace event sample Li Zefan
2009-05-06 2:32 ` [PATCH 2/4] tracing/events: make SAMPLE_TRACE_EVENTS default to n Li Zefan
2009-05-06 2:33 ` [PATCH 3/4] tracing/events: fix memory leak when unloading module Li Zefan
@ 2009-05-06 2:33 ` Li Zefan
2009-05-06 5:27 ` Frederic Weisbecker
` (2 more replies)
2009-05-06 2:41 ` [PATCH 1/4] tracing/events: don't say hi when loading the trace event sample Steven Rostedt
2009-05-06 12:19 ` [tip:tracing/core] " tip-bot for Li Zefan
4 siblings, 3 replies; 16+ messages in thread
From: Li Zefan @ 2009-05-06 2:33 UTC (permalink / raw)
To: Steven Rostedt
Cc: Frederic Weisbecker, Ingo Molnar, LKML, Tom Zanussi, Peter Zijlstra
A module will add/remove its trace events when it gets loaded/unloaded, so
the ftrace_events list is not "const", and concurrent access needs to be
protected.
This patch thus fixes races between loading/unloding modules and read
'available_events' or read/write 'set_event', etc.
Below shows how to reproduce the race:
# for ((; ;)) { cat /mnt/tracing/available_events; } > /dev/null &
# for ((; ;)) { insmod trace-events-sample.ko; rmmod sample; } &
After a while:
BUG: unable to handle kernel paging request at 0010011c
IP: [<c1080f27>] t_next+0x1b/0x2d
...
Call Trace:
[<c10c90e6>] ? seq_read+0x217/0x30d
[<c10c8ecf>] ? seq_read+0x0/0x30d
[<c10b4c19>] ? vfs_read+0x8f/0x136
[<c10b4fc3>] ? sys_read+0x40/0x65
[<c1002a68>] ? sysenter_do_call+0x12/0x36
[ Impact: fix races when concurrent accessing ftrace_events list ]
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
kernel/trace/trace.h | 1 +
kernel/trace/trace_event_profile.c | 19 ++++++++++++++-----
kernel/trace/trace_events.c | 20 +++++++++++---------
kernel/trace/trace_events_filter.c | 10 +++++++---
4 files changed, 33 insertions(+), 17 deletions(-)
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 83984ab..0bba080 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -827,6 +827,7 @@ static int filter_pred_##size(struct filter_pred *pred, void *event, \
return match; \
}
+extern struct mutex event_mutex;
extern struct list_head ftrace_events;
extern const char *__start___trace_bprintk_fmt[];
diff --git a/kernel/trace/trace_event_profile.c b/kernel/trace/trace_event_profile.c
index 7bf2ad6..5b5895a 100644
--- a/kernel/trace/trace_event_profile.c
+++ b/kernel/trace/trace_event_profile.c
@@ -10,21 +10,30 @@
int ftrace_profile_enable(int event_id)
{
struct ftrace_event_call *event;
+ int ret = -EINVAL;
+ mutex_lock(&event_mutex);
list_for_each_entry(event, &ftrace_events, list) {
- if (event->id == event_id)
- return event->profile_enable(event);
+ if (event->id == event_id) {
+ ret = event->profile_enable(event);
+ break;
+ }
}
+ mutex_unlock(&event_mutex);
- return -EINVAL;
+ return ret;
}
void ftrace_profile_disable(int event_id)
{
struct ftrace_event_call *event;
+ mutex_lock(&event_mutex);
list_for_each_entry(event, &ftrace_events, list) {
- if (event->id == event_id)
- return event->profile_disable(event);
+ if (event->id == event_id) {
+ event->profile_disable(event);
+ break;
+ }
}
+ mutex_unlock(&event_mutex);
}
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 72d619d..ac5b1c2 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -21,7 +21,7 @@
#define TRACE_SYSTEM "TRACE_SYSTEM"
-static DEFINE_MUTEX(event_mutex);
+DEFINE_MUTEX(event_mutex);
LIST_HEAD(ftrace_events);
@@ -80,6 +80,7 @@ static void ftrace_clear_events(void)
{
struct ftrace_event_call *call;
+ mutex_lock(&event_mutex);
list_for_each_entry(call, &ftrace_events, list) {
if (call->enabled) {
@@ -87,6 +88,7 @@ static void ftrace_clear_events(void)
call->unregfunc();
}
}
+ mutex_unlock(&event_mutex);
}
static void ftrace_event_enable_disable(struct ftrace_event_call *call,
@@ -274,6 +276,9 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
static void *t_start(struct seq_file *m, loff_t *pos)
{
+ mutex_lock(&event_mutex);
+ if (*pos == 0)
+ m->private = ftrace_events.next;
return t_next(m, NULL, pos);
}
@@ -303,6 +308,9 @@ s_next(struct seq_file *m, void *v, loff_t *pos)
static void *s_start(struct seq_file *m, loff_t *pos)
{
+ mutex_lock(&event_mutex);
+ if (*pos == 0)
+ m->private = ftrace_events.next;
return s_next(m, NULL, pos);
}
@@ -319,12 +327,12 @@ static int t_show(struct seq_file *m, void *v)
static void t_stop(struct seq_file *m, void *p)
{
+ mutex_unlock(&event_mutex);
}
static int
ftrace_event_seq_open(struct inode *inode, struct file *file)
{
- int ret;
const struct seq_operations *seq_ops;
if ((file->f_mode & FMODE_WRITE) &&
@@ -332,13 +340,7 @@ ftrace_event_seq_open(struct inode *inode, struct file *file)
ftrace_clear_events();
seq_ops = inode->i_private;
- ret = seq_open(file, seq_ops);
- if (!ret) {
- struct seq_file *m = file->private_data;
-
- m->private = ftrace_events.next;
- }
- return ret;
+ return seq_open(file, seq_ops);
}
static ssize_t
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 97bd140..8c62e5b 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -433,6 +433,7 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
filter->n_preds = 0;
}
+ mutex_lock(&event_mutex);
list_for_each_entry(call, &ftrace_events, list) {
if (!call->define_fields)
continue;
@@ -442,6 +443,7 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
remove_filter_string(call->filter);
}
}
+ mutex_unlock(&event_mutex);
}
static int filter_add_pred_fn(struct filter_parse_state *ps,
@@ -605,6 +607,7 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
{
struct event_filter *filter = system->filter;
struct ftrace_event_call *call;
+ int err = 0;
if (!filter->preds) {
filter->preds = kzalloc(MAX_FILTER_PRED * sizeof(pred),
@@ -622,8 +625,8 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
filter->preds[filter->n_preds] = pred;
filter->n_preds++;
+ mutex_lock(&event_mutex);
list_for_each_entry(call, &ftrace_events, list) {
- int err;
if (!call->define_fields)
continue;
@@ -635,12 +638,13 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
if (err) {
filter_free_subsystem_preds(system);
parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);
- return err;
+ break;
}
replace_filter_string(call->filter, filter_string);
}
+ mutex_unlock(&event_mutex);
- return 0;
+ return err;
}
static void parse_init(struct filter_parse_state *ps,
--
1.5.4.rc3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] tracing/events: don't say hi when loading the trace event sample
2009-05-06 2:32 [PATCH 1/4] tracing/events: don't say hi when loading the trace event sample Li Zefan
` (2 preceding siblings ...)
2009-05-06 2:33 ` [PATCH 4/4] tracing/events: fix concurrent access to ftrace_events list Li Zefan
@ 2009-05-06 2:41 ` Steven Rostedt
2009-05-06 12:19 ` [tip:tracing/core] " tip-bot for Li Zefan
4 siblings, 0 replies; 16+ messages in thread
From: Steven Rostedt @ 2009-05-06 2:41 UTC (permalink / raw)
To: Li Zefan; +Cc: Frederic Weisbecker, Ingo Molnar, LKML
On Wed, 6 May 2009, Li Zefan wrote:
> The sample is useful for testing, and I'm using it. But after
> loading the module, it keeps saying hi every 10 seconds, this may
> be disturbing.
>
> Also Steven said commenting out the "hi" helped in causing races. :)
>
> [ Impact: make testing a bit easier ]
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
> samples/trace_events/trace-events-sample.c | 4 ----
> 1 files changed, 0 insertions(+), 4 deletions(-)
>
> diff --git a/samples/trace_events/trace-events-sample.c b/samples/trace_events/trace-events-sample.c
> index f33b3ba..aabc4e9 100644
> --- a/samples/trace_events/trace-events-sample.c
> +++ b/samples/trace_events/trace-events-sample.c
> @@ -16,10 +16,6 @@ static void simple_thread_func(int cnt)
> set_current_state(TASK_INTERRUPTIBLE);
> schedule_timeout(HZ);
> trace_foo_bar("hello", cnt);
> -
> - if (!(cnt % 10))
> - /* It is really important that I say "hi!" */
I guess it is not so important to say hi after all.
-- Steve
> - printk(KERN_EMERG "hi!\n");
> }
>
> static int simple_thread(void *arg)
> --
> 1.5.4.rc3
>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] tracing/events: fix memory leak when unloading module
2009-05-06 2:33 ` [PATCH 3/4] tracing/events: fix memory leak when unloading module Li Zefan
@ 2009-05-06 2:43 ` Steven Rostedt
2009-05-06 12:19 ` [tip:tracing/core] " tip-bot for Li Zefan
1 sibling, 0 replies; 16+ messages in thread
From: Steven Rostedt @ 2009-05-06 2:43 UTC (permalink / raw)
To: Li Zefan; +Cc: Frederic Weisbecker, Ingo Molnar, LKML, Tom Zanussi
On Wed, 6 May 2009, Li Zefan wrote:
> When unloading a module, memory allocated by init_preds() and
> trace_define_field() is not freed.
>
> [ Impact: fix memory leak ]
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Nice catch!
Acked-by: Steven Rostedt <rostedt@goodmis.org>
-- Steve
> ---
> include/linux/ftrace_event.h | 1 +
> kernel/trace/trace_events.c | 18 ++++++++++++++++++
> kernel/trace/trace_events_filter.c | 22 +++++++++++++++-------
> 3 files changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 5fff40c..662c1be 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -116,6 +116,7 @@ struct ftrace_event_call {
> #define MAX_FILTER_STR_VAL 128
>
> extern int init_preds(struct ftrace_event_call *call);
> +extern void destroy_preds(struct ftrace_event_call *call);
> extern int filter_match_preds(struct ftrace_event_call *call, void *rec);
> extern int filter_current_check_discard(struct ftrace_event_call *call,
> void *rec,
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 9998cc8..72d619d 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -60,6 +60,22 @@ err:
> }
> EXPORT_SYMBOL_GPL(trace_define_field);
>
> +#ifdef CONFIG_MODULES
> +
> +static void trace_destroy_fields(struct ftrace_event_call *call)
> +{
> + struct ftrace_event_field *field, *next;
> +
> + list_for_each_entry_safe(field, next, &call->fields, link) {
> + list_del(&field->link);
> + kfree(field->type);
> + kfree(field->name);
> + kfree(field);
> + }
> +}
> +
> +#endif /* CONFIG_MODULES */
> +
> static void ftrace_clear_events(void)
> {
> struct ftrace_event_call *call;
> @@ -925,6 +941,8 @@ static void trace_module_remove_events(struct module *mod)
> unregister_ftrace_event(call->event);
> debugfs_remove_recursive(call->dir);
> list_del(&call->list);
> + trace_destroy_fields(call);
> + destroy_preds(call);
> }
> }
>
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index ac30de3..97bd140 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -371,6 +371,20 @@ static void filter_disable_preds(struct ftrace_event_call *call)
> filter->preds[i]->fn = filter_pred_none;
> }
>
> +void destroy_preds(struct ftrace_event_call *call)
> +{
> + struct event_filter *filter = call->filter;
> + int i;
> +
> + for (i = 0; i < MAX_FILTER_PRED; i++) {
> + if (filter->preds[i])
> + filter_free_pred(filter->preds[i]);
> + }
> + kfree(filter->preds);
> + kfree(filter);
> + call->filter = NULL;
> +}
> +
> int init_preds(struct ftrace_event_call *call)
> {
> struct event_filter *filter;
> @@ -399,13 +413,7 @@ int init_preds(struct ftrace_event_call *call)
> return 0;
>
> oom:
> - for (i = 0; i < MAX_FILTER_PRED; i++) {
> - if (filter->preds[i])
> - filter_free_pred(filter->preds[i]);
> - }
> - kfree(filter->preds);
> - kfree(call->filter);
> - call->filter = NULL;
> + destroy_preds(call);
>
> return -ENOMEM;
> }
> --
> 1.5.4.rc3
>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/4] tracing/events: fix concurrent access to ftrace_events list
2009-05-06 2:33 ` [PATCH 4/4] tracing/events: fix concurrent access to ftrace_events list Li Zefan
@ 2009-05-06 5:27 ` Frederic Weisbecker
2009-05-06 8:40 ` Ingo Molnar
2009-05-06 5:30 ` [PATCH v2] " Li Zefan
2009-05-06 12:19 ` [tip:tracing/core] " tip-bot for Li Zefan
2 siblings, 1 reply; 16+ messages in thread
From: Frederic Weisbecker @ 2009-05-06 5:27 UTC (permalink / raw)
To: Li Zefan; +Cc: Steven Rostedt, Ingo Molnar, LKML, Tom Zanussi, Peter Zijlstra
On Wed, May 06, 2009 at 10:33:45AM +0800, Li Zefan wrote:
> A module will add/remove its trace events when it gets loaded/unloaded, so
> the ftrace_events list is not "const", and concurrent access needs to be
> protected.
>
> This patch thus fixes races between loading/unloding modules and read
> 'available_events' or read/write 'set_event', etc.
>
> Below shows how to reproduce the race:
>
> # for ((; ;)) { cat /mnt/tracing/available_events; } > /dev/null &
> # for ((; ;)) { insmod trace-events-sample.ko; rmmod sample; } &
>
> After a while:
>
> BUG: unable to handle kernel paging request at 0010011c
> IP: [<c1080f27>] t_next+0x1b/0x2d
> ...
> Call Trace:
> [<c10c90e6>] ? seq_read+0x217/0x30d
> [<c10c8ecf>] ? seq_read+0x0/0x30d
> [<c10b4c19>] ? vfs_read+0x8f/0x136
> [<c10b4fc3>] ? sys_read+0x40/0x65
> [<c1002a68>] ? sysenter_do_call+0x12/0x36
>
> [ Impact: fix races when concurrent accessing ftrace_events list ]
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Nice series of fixes!
Thanks!
Frederic.
> ---
> kernel/trace/trace.h | 1 +
> kernel/trace/trace_event_profile.c | 19 ++++++++++++++-----
> kernel/trace/trace_events.c | 20 +++++++++++---------
> kernel/trace/trace_events_filter.c | 10 +++++++---
> 4 files changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 83984ab..0bba080 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -827,6 +827,7 @@ static int filter_pred_##size(struct filter_pred *pred, void *event, \
> return match; \
> }
>
> +extern struct mutex event_mutex;
> extern struct list_head ftrace_events;
>
> extern const char *__start___trace_bprintk_fmt[];
> diff --git a/kernel/trace/trace_event_profile.c b/kernel/trace/trace_event_profile.c
> index 7bf2ad6..5b5895a 100644
> --- a/kernel/trace/trace_event_profile.c
> +++ b/kernel/trace/trace_event_profile.c
> @@ -10,21 +10,30 @@
> int ftrace_profile_enable(int event_id)
> {
> struct ftrace_event_call *event;
> + int ret = -EINVAL;
>
> + mutex_lock(&event_mutex);
> list_for_each_entry(event, &ftrace_events, list) {
> - if (event->id == event_id)
> - return event->profile_enable(event);
> + if (event->id == event_id) {
> + ret = event->profile_enable(event);
> + break;
> + }
> }
> + mutex_unlock(&event_mutex);
>
> - return -EINVAL;
> + return ret;
> }
>
> void ftrace_profile_disable(int event_id)
> {
> struct ftrace_event_call *event;
>
> + mutex_lock(&event_mutex);
> list_for_each_entry(event, &ftrace_events, list) {
> - if (event->id == event_id)
> - return event->profile_disable(event);
> + if (event->id == event_id) {
> + event->profile_disable(event);
> + break;
> + }
> }
> + mutex_unlock(&event_mutex);
> }
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 72d619d..ac5b1c2 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -21,7 +21,7 @@
>
> #define TRACE_SYSTEM "TRACE_SYSTEM"
>
> -static DEFINE_MUTEX(event_mutex);
> +DEFINE_MUTEX(event_mutex);
>
> LIST_HEAD(ftrace_events);
>
> @@ -80,6 +80,7 @@ static void ftrace_clear_events(void)
> {
> struct ftrace_event_call *call;
>
> + mutex_lock(&event_mutex);
> list_for_each_entry(call, &ftrace_events, list) {
>
> if (call->enabled) {
> @@ -87,6 +88,7 @@ static void ftrace_clear_events(void)
> call->unregfunc();
> }
> }
> + mutex_unlock(&event_mutex);
> }
>
> static void ftrace_event_enable_disable(struct ftrace_event_call *call,
> @@ -274,6 +276,9 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
>
> static void *t_start(struct seq_file *m, loff_t *pos)
> {
> + mutex_lock(&event_mutex);
> + if (*pos == 0)
> + m->private = ftrace_events.next;
> return t_next(m, NULL, pos);
> }
>
> @@ -303,6 +308,9 @@ s_next(struct seq_file *m, void *v, loff_t *pos)
>
> static void *s_start(struct seq_file *m, loff_t *pos)
> {
> + mutex_lock(&event_mutex);
> + if (*pos == 0)
> + m->private = ftrace_events.next;
> return s_next(m, NULL, pos);
> }
>
> @@ -319,12 +327,12 @@ static int t_show(struct seq_file *m, void *v)
>
> static void t_stop(struct seq_file *m, void *p)
> {
> + mutex_unlock(&event_mutex);
> }
>
> static int
> ftrace_event_seq_open(struct inode *inode, struct file *file)
> {
> - int ret;
> const struct seq_operations *seq_ops;
>
> if ((file->f_mode & FMODE_WRITE) &&
> @@ -332,13 +340,7 @@ ftrace_event_seq_open(struct inode *inode, struct file *file)
> ftrace_clear_events();
>
> seq_ops = inode->i_private;
> - ret = seq_open(file, seq_ops);
> - if (!ret) {
> - struct seq_file *m = file->private_data;
> -
> - m->private = ftrace_events.next;
> - }
> - return ret;
> + return seq_open(file, seq_ops);
> }
>
> static ssize_t
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 97bd140..8c62e5b 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -433,6 +433,7 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
> filter->n_preds = 0;
> }
>
> + mutex_lock(&event_mutex);
> list_for_each_entry(call, &ftrace_events, list) {
> if (!call->define_fields)
> continue;
> @@ -442,6 +443,7 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
> remove_filter_string(call->filter);
> }
> }
> + mutex_unlock(&event_mutex);
> }
>
> static int filter_add_pred_fn(struct filter_parse_state *ps,
> @@ -605,6 +607,7 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
> {
> struct event_filter *filter = system->filter;
> struct ftrace_event_call *call;
> + int err = 0;
>
> if (!filter->preds) {
> filter->preds = kzalloc(MAX_FILTER_PRED * sizeof(pred),
> @@ -622,8 +625,8 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
> filter->preds[filter->n_preds] = pred;
> filter->n_preds++;
>
> + mutex_lock(&event_mutex);
> list_for_each_entry(call, &ftrace_events, list) {
> - int err;
>
> if (!call->define_fields)
> continue;
> @@ -635,12 +638,13 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
> if (err) {
> filter_free_subsystem_preds(system);
> parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);
> - return err;
> + break;
> }
> replace_filter_string(call->filter, filter_string);
> }
> + mutex_unlock(&event_mutex);
>
> - return 0;
> + return err;
> }
>
> static void parse_init(struct filter_parse_state *ps,
> --
> 1.5.4.rc3
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] tracing/events: fix concurrent access to ftrace_events list
2009-05-06 2:33 ` [PATCH 4/4] tracing/events: fix concurrent access to ftrace_events list Li Zefan
2009-05-06 5:27 ` Frederic Weisbecker
@ 2009-05-06 5:30 ` Li Zefan
2009-05-06 12:19 ` [tip:tracing/core] " tip-bot for Li Zefan
2 siblings, 0 replies; 16+ messages in thread
From: Li Zefan @ 2009-05-06 5:30 UTC (permalink / raw)
To: Steven Rostedt
Cc: Frederic Weisbecker, Ingo Molnar, LKML, Tom Zanussi, Peter Zijlstra
A module will add/remove its trace events when it gets loaded/unloaded, so
the ftrace_events list is not "const", and concurrent access needs to be
protected.
This patch thus fixes races between loading/unloding modules and read
'available_events' or read/write 'set_event', etc.
Below shows how to reproduce the race:
# for ((; ;)) { cat /mnt/tracing/available_events; } > /dev/null &
# for ((; ;)) { insmod trace-events-sample.ko; rmmod sample; } &
After a while:
BUG: unable to handle kernel paging request at 0010011c
IP: [<c1080f27>] t_next+0x1b/0x2d
...
Call Trace:
[<c10c90e6>] ? seq_read+0x217/0x30d
[<c10c8ecf>] ? seq_read+0x0/0x30d
[<c10b4c19>] ? vfs_read+0x8f/0x136
[<c10b4fc3>] ? sys_read+0x40/0x65
[<c1002a68>] ? sysenter_do_call+0x12/0x36
Changelog v1 -> v2:
- Fix deadlock when writing invalid pred into subsystem filter.
[ Impact: fix races when concurrent accessing ftrace_events list ]
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
kernel/trace/trace.h | 1 +
kernel/trace/trace_event_profile.c | 19 ++++++++++++++-----
kernel/trace/trace_events.c | 20 +++++++++++---------
kernel/trace/trace_events_filter.c | 5 +++++
4 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 83984ab..0bba080 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -827,6 +827,7 @@ static int filter_pred_##size(struct filter_pred *pred, void *event, \
return match; \
}
+extern struct mutex event_mutex;
extern struct list_head ftrace_events;
extern const char *__start___trace_bprintk_fmt[];
diff --git a/kernel/trace/trace_event_profile.c b/kernel/trace/trace_event_profile.c
index 7bf2ad6..5b5895a 100644
--- a/kernel/trace/trace_event_profile.c
+++ b/kernel/trace/trace_event_profile.c
@@ -10,21 +10,30 @@
int ftrace_profile_enable(int event_id)
{
struct ftrace_event_call *event;
+ int ret = -EINVAL;
+ mutex_lock(&event_mutex);
list_for_each_entry(event, &ftrace_events, list) {
- if (event->id == event_id)
- return event->profile_enable(event);
+ if (event->id == event_id) {
+ ret = event->profile_enable(event);
+ break;
+ }
}
+ mutex_unlock(&event_mutex);
- return -EINVAL;
+ return ret;
}
void ftrace_profile_disable(int event_id)
{
struct ftrace_event_call *event;
+ mutex_lock(&event_mutex);
list_for_each_entry(event, &ftrace_events, list) {
- if (event->id == event_id)
- return event->profile_disable(event);
+ if (event->id == event_id) {
+ event->profile_disable(event);
+ break;
+ }
}
+ mutex_unlock(&event_mutex);
}
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 72d619d..ac5b1c2 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -21,7 +21,7 @@
#define TRACE_SYSTEM "TRACE_SYSTEM"
-static DEFINE_MUTEX(event_mutex);
+DEFINE_MUTEX(event_mutex);
LIST_HEAD(ftrace_events);
@@ -80,6 +80,7 @@ static void ftrace_clear_events(void)
{
struct ftrace_event_call *call;
+ mutex_lock(&event_mutex);
list_for_each_entry(call, &ftrace_events, list) {
if (call->enabled) {
@@ -87,6 +88,7 @@ static void ftrace_clear_events(void)
call->unregfunc();
}
}
+ mutex_unlock(&event_mutex);
}
static void ftrace_event_enable_disable(struct ftrace_event_call *call,
@@ -274,6 +276,9 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
static void *t_start(struct seq_file *m, loff_t *pos)
{
+ mutex_lock(&event_mutex);
+ if (*pos == 0)
+ m->private = ftrace_events.next;
return t_next(m, NULL, pos);
}
@@ -303,6 +308,9 @@ s_next(struct seq_file *m, void *v, loff_t *pos)
static void *s_start(struct seq_file *m, loff_t *pos)
{
+ mutex_lock(&event_mutex);
+ if (*pos == 0)
+ m->private = ftrace_events.next;
return s_next(m, NULL, pos);
}
@@ -319,12 +327,12 @@ static int t_show(struct seq_file *m, void *v)
static void t_stop(struct seq_file *m, void *p)
{
+ mutex_unlock(&event_mutex);
}
static int
ftrace_event_seq_open(struct inode *inode, struct file *file)
{
- int ret;
const struct seq_operations *seq_ops;
if ((file->f_mode & FMODE_WRITE) &&
@@ -332,13 +340,7 @@ ftrace_event_seq_open(struct inode *inode, struct file *file)
ftrace_clear_events();
seq_ops = inode->i_private;
- ret = seq_open(file, seq_ops);
- if (!ret) {
- struct seq_file *m = file->private_data;
-
- m->private = ftrace_events.next;
- }
- return ret;
+ return seq_open(file, seq_ops);
}
static ssize_t
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 97bd140..de1f821 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -433,6 +433,7 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
filter->n_preds = 0;
}
+ mutex_lock(&event_mutex);
list_for_each_entry(call, &ftrace_events, list) {
if (!call->define_fields)
continue;
@@ -442,6 +443,7 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
remove_filter_string(call->filter);
}
}
+ mutex_unlock(&event_mutex);
}
static int filter_add_pred_fn(struct filter_parse_state *ps,
@@ -622,6 +624,7 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
filter->preds[filter->n_preds] = pred;
filter->n_preds++;
+ mutex_lock(&event_mutex);
list_for_each_entry(call, &ftrace_events, list) {
int err;
@@ -633,12 +636,14 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
err = filter_add_pred(ps, call, pred);
if (err) {
+ mutex_unlock(&event_mutex);
filter_free_subsystem_preds(system);
parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);
return err;
}
replace_filter_string(call->filter, filter_string);
}
+ mutex_unlock(&event_mutex);
return 0;
}
--
1.5.4.rc3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 4/4] tracing/events: fix concurrent access to ftrace_events list
2009-05-06 5:27 ` Frederic Weisbecker
@ 2009-05-06 8:40 ` Ingo Molnar
0 siblings, 0 replies; 16+ messages in thread
From: Ingo Molnar @ 2009-05-06 8:40 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: Li Zefan, Steven Rostedt, LKML, Tom Zanussi, Peter Zijlstra
* Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Wed, May 06, 2009 at 10:33:45AM +0800, Li Zefan wrote:
> > A module will add/remove its trace events when it gets loaded/unloaded, so
> > the ftrace_events list is not "const", and concurrent access needs to be
> > protected.
> >
> > This patch thus fixes races between loading/unloding modules and read
> > 'available_events' or read/write 'set_event', etc.
> >
> > Below shows how to reproduce the race:
> >
> > # for ((; ;)) { cat /mnt/tracing/available_events; } > /dev/null &
> > # for ((; ;)) { insmod trace-events-sample.ko; rmmod sample; } &
> >
> > After a while:
> >
> > BUG: unable to handle kernel paging request at 0010011c
> > IP: [<c1080f27>] t_next+0x1b/0x2d
> > ...
> > Call Trace:
> > [<c10c90e6>] ? seq_read+0x217/0x30d
> > [<c10c8ecf>] ? seq_read+0x0/0x30d
> > [<c10b4c19>] ? vfs_read+0x8f/0x136
> > [<c10b4fc3>] ? sys_read+0x40/0x65
> > [<c1002a68>] ? sysenter_do_call+0x12/0x36
> >
> > [ Impact: fix races when concurrent accessing ftrace_events list ]
> >
> > Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>
>
> Nice series of fixes!
>
> Thanks!
I've applied them to tip/tracing/events, thanks guys!
Ingo
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tip:tracing/core] tracing/events: don't say hi when loading the trace event sample
2009-05-06 2:32 [PATCH 1/4] tracing/events: don't say hi when loading the trace event sample Li Zefan
` (3 preceding siblings ...)
2009-05-06 2:41 ` [PATCH 1/4] tracing/events: don't say hi when loading the trace event sample Steven Rostedt
@ 2009-05-06 12:19 ` tip-bot for Li Zefan
4 siblings, 0 replies; 16+ messages in thread
From: tip-bot for Li Zefan @ 2009-05-06 12:19 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, fweisbec, rostedt, lizf, tglx, mingo
Commit-ID: fd6da10a617f483348ee32bcfe53fd20c302eca1
Gitweb: http://git.kernel.org/tip/fd6da10a617f483348ee32bcfe53fd20c302eca1
Author: Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Wed, 6 May 2009 10:32:13 +0800
Committer: Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 6 May 2009 10:38:18 +0200
tracing/events: don't say hi when loading the trace event sample
The sample is useful for testing, and I'm using it. But after
loading the module, it keeps saying hi every 10 seconds, this may
be disturbing.
Also Steven said commenting out the "hi" helped in causing races. :)
[ Impact: make testing a bit easier ]
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
LKML-Reference: <4A00F6AD.2070008@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
samples/trace_events/trace-events-sample.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/samples/trace_events/trace-events-sample.c b/samples/trace_events/trace-events-sample.c
index f33b3ba..aabc4e9 100644
--- a/samples/trace_events/trace-events-sample.c
+++ b/samples/trace_events/trace-events-sample.c
@@ -16,10 +16,6 @@ static void simple_thread_func(int cnt)
set_current_state(TASK_INTERRUPTIBLE);
schedule_timeout(HZ);
trace_foo_bar("hello", cnt);
-
- if (!(cnt % 10))
- /* It is really important that I say "hi!" */
- printk(KERN_EMERG "hi!\n");
}
static int simple_thread(void *arg)
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [tip:tracing/core] tracing/events: make SAMPLE_TRACE_EVENTS default to n
2009-05-06 2:32 ` [PATCH 2/4] tracing/events: make SAMPLE_TRACE_EVENTS default to n Li Zefan
@ 2009-05-06 12:19 ` tip-bot for Li Zefan
0 siblings, 0 replies; 16+ messages in thread
From: tip-bot for Li Zefan @ 2009-05-06 12:19 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, fweisbec, rostedt, lizf, tglx, mingo
Commit-ID: 96d17980fabeb757706d2d6db5a28580a6156bfc
Gitweb: http://git.kernel.org/tip/96d17980fabeb757706d2d6db5a28580a6156bfc
Author: Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Wed, 6 May 2009 10:32:32 +0800
Committer: Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 6 May 2009 10:38:18 +0200
tracing/events: make SAMPLE_TRACE_EVENTS default to n
Normally a config should be default to n. This patch also makes the
sample module-only, like SAMPLE_MARKERS and SAMPLE_TRACEPOINTS.
[ Impact: don't build trace event sample by default ]
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
LKML-Reference: <4A00F6C0.8090803@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
samples/Kconfig | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/samples/Kconfig b/samples/Kconfig
index 93f41c0..b75d28c 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -20,9 +20,8 @@ config SAMPLE_TRACEPOINTS
This build tracepoints example modules.
config SAMPLE_TRACE_EVENTS
- tristate "Build trace_events examples"
- depends on EVENT_TRACING
- default m
+ tristate "Build trace_events examples -- loadable modules only"
+ depends on EVENT_TRACING && m
help
This build trace event example modules.
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [tip:tracing/core] tracing/events: fix memory leak when unloading module
2009-05-06 2:33 ` [PATCH 3/4] tracing/events: fix memory leak when unloading module Li Zefan
2009-05-06 2:43 ` Steven Rostedt
@ 2009-05-06 12:19 ` tip-bot for Li Zefan
1 sibling, 0 replies; 16+ messages in thread
From: tip-bot for Li Zefan @ 2009-05-06 12:19 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, tzanussi, lizf, fweisbec, rostedt, tglx, mingo
Commit-ID: 2df75e415709ad12862028916c772c1f377f6a7c
Gitweb: http://git.kernel.org/tip/2df75e415709ad12862028916c772c1f377f6a7c
Author: Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Wed, 6 May 2009 10:33:04 +0800
Committer: Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 6 May 2009 10:38:19 +0200
tracing/events: fix memory leak when unloading module
When unloading a module, memory allocated by init_preds() and
trace_define_field() is not freed.
[ Impact: fix memory leak ]
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Tom Zanussi <tzanussi@gmail.com>
LKML-Reference: <4A00F6E0.3040503@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
include/linux/ftrace_event.h | 1 +
kernel/trace/trace_events.c | 18 ++++++++++++++++++
kernel/trace/trace_events_filter.c | 22 +++++++++++++++-------
3 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 5fff40c..662c1be 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -116,6 +116,7 @@ struct ftrace_event_call {
#define MAX_FILTER_STR_VAL 128
extern int init_preds(struct ftrace_event_call *call);
+extern void destroy_preds(struct ftrace_event_call *call);
extern int filter_match_preds(struct ftrace_event_call *call, void *rec);
extern int filter_current_check_discard(struct ftrace_event_call *call,
void *rec,
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index f789ca5..f251a15 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -60,6 +60,22 @@ err:
}
EXPORT_SYMBOL_GPL(trace_define_field);
+#ifdef CONFIG_MODULES
+
+static void trace_destroy_fields(struct ftrace_event_call *call)
+{
+ struct ftrace_event_field *field, *next;
+
+ list_for_each_entry_safe(field, next, &call->fields, link) {
+ list_del(&field->link);
+ kfree(field->type);
+ kfree(field->name);
+ kfree(field);
+ }
+}
+
+#endif /* CONFIG_MODULES */
+
static void ftrace_clear_events(void)
{
struct ftrace_event_call *call;
@@ -925,6 +941,8 @@ static void trace_module_remove_events(struct module *mod)
unregister_ftrace_event(call->event);
debugfs_remove_recursive(call->dir);
list_del(&call->list);
+ trace_destroy_fields(call);
+ destroy_preds(call);
}
}
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index f494866..ce07b81 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -346,6 +346,20 @@ static void filter_disable_preds(struct ftrace_event_call *call)
filter->preds[i]->fn = filter_pred_none;
}
+void destroy_preds(struct ftrace_event_call *call)
+{
+ struct event_filter *filter = call->filter;
+ int i;
+
+ for (i = 0; i < MAX_FILTER_PRED; i++) {
+ if (filter->preds[i])
+ filter_free_pred(filter->preds[i]);
+ }
+ kfree(filter->preds);
+ kfree(filter);
+ call->filter = NULL;
+}
+
int init_preds(struct ftrace_event_call *call)
{
struct event_filter *filter;
@@ -374,13 +388,7 @@ int init_preds(struct ftrace_event_call *call)
return 0;
oom:
- for (i = 0; i < MAX_FILTER_PRED; i++) {
- if (filter->preds[i])
- filter_free_pred(filter->preds[i]);
- }
- kfree(filter->preds);
- kfree(call->filter);
- call->filter = NULL;
+ destroy_preds(call);
return -ENOMEM;
}
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [tip:tracing/core] tracing/events: fix concurrent access to ftrace_events list
2009-05-06 2:33 ` [PATCH 4/4] tracing/events: fix concurrent access to ftrace_events list Li Zefan
2009-05-06 5:27 ` Frederic Weisbecker
2009-05-06 5:30 ` [PATCH v2] " Li Zefan
@ 2009-05-06 12:19 ` tip-bot for Li Zefan
2009-05-07 7:11 ` Li Zefan
2 siblings, 1 reply; 16+ messages in thread
From: tip-bot for Li Zefan @ 2009-05-06 12:19 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, tzanussi, a.p.zijlstra, lizf, fweisbec,
rostedt, tglx, mingo
Commit-ID: 20c8928abe70e204bd077ab6cfe23002d7788983
Gitweb: http://git.kernel.org/tip/20c8928abe70e204bd077ab6cfe23002d7788983
Author: Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Wed, 6 May 2009 10:33:45 +0800
Committer: Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 6 May 2009 10:38:19 +0200
tracing/events: fix concurrent access to ftrace_events list
A module will add/remove its trace events when it gets loaded/unloaded, so
the ftrace_events list is not "const", and concurrent access needs to be
protected.
This patch thus fixes races between loading/unloding modules and read
'available_events' or read/write 'set_event', etc.
Below shows how to reproduce the race:
# for ((; ;)) { cat /mnt/tracing/available_events; } > /dev/null &
# for ((; ;)) { insmod trace-events-sample.ko; rmmod sample; } &
After a while:
BUG: unable to handle kernel paging request at 0010011c
IP: [<c1080f27>] t_next+0x1b/0x2d
..
Call Trace:
[<c10c90e6>] ? seq_read+0x217/0x30d
[<c10c8ecf>] ? seq_read+0x0/0x30d
[<c10b4c19>] ? vfs_read+0x8f/0x136
[<c10b4fc3>] ? sys_read+0x40/0x65
[<c1002a68>] ? sysenter_do_call+0x12/0x36
[ Impact: fix races when concurrent accessing ftrace_events list ]
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Tom Zanussi <tzanussi@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <4A00F709.3080800@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
kernel/trace/trace.h | 1 +
kernel/trace/trace_event_profile.c | 19 ++++++++++++++-----
kernel/trace/trace_events.c | 20 +++++++++++---------
kernel/trace/trace_events_filter.c | 10 +++++++---
4 files changed, 33 insertions(+), 17 deletions(-)
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 7736fe8..777c6c3 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -825,6 +825,7 @@ static int filter_pred_##size(struct filter_pred *pred, void *event, \
return match; \
}
+extern struct mutex event_mutex;
extern struct list_head ftrace_events;
extern const char *__start___trace_bprintk_fmt[];
diff --git a/kernel/trace/trace_event_profile.c b/kernel/trace/trace_event_profile.c
index 7bf2ad6..5b5895a 100644
--- a/kernel/trace/trace_event_profile.c
+++ b/kernel/trace/trace_event_profile.c
@@ -10,21 +10,30 @@
int ftrace_profile_enable(int event_id)
{
struct ftrace_event_call *event;
+ int ret = -EINVAL;
+ mutex_lock(&event_mutex);
list_for_each_entry(event, &ftrace_events, list) {
- if (event->id == event_id)
- return event->profile_enable(event);
+ if (event->id == event_id) {
+ ret = event->profile_enable(event);
+ break;
+ }
}
+ mutex_unlock(&event_mutex);
- return -EINVAL;
+ return ret;
}
void ftrace_profile_disable(int event_id)
{
struct ftrace_event_call *event;
+ mutex_lock(&event_mutex);
list_for_each_entry(event, &ftrace_events, list) {
- if (event->id == event_id)
- return event->profile_disable(event);
+ if (event->id == event_id) {
+ event->profile_disable(event);
+ break;
+ }
}
+ mutex_unlock(&event_mutex);
}
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index f251a15..8d579ff 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -21,7 +21,7 @@
#define TRACE_SYSTEM "TRACE_SYSTEM"
-static DEFINE_MUTEX(event_mutex);
+DEFINE_MUTEX(event_mutex);
LIST_HEAD(ftrace_events);
@@ -80,6 +80,7 @@ static void ftrace_clear_events(void)
{
struct ftrace_event_call *call;
+ mutex_lock(&event_mutex);
list_for_each_entry(call, &ftrace_events, list) {
if (call->enabled) {
@@ -87,6 +88,7 @@ static void ftrace_clear_events(void)
call->unregfunc();
}
}
+ mutex_unlock(&event_mutex);
}
static void ftrace_event_enable_disable(struct ftrace_event_call *call,
@@ -274,6 +276,9 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
static void *t_start(struct seq_file *m, loff_t *pos)
{
+ mutex_lock(&event_mutex);
+ if (*pos == 0)
+ m->private = ftrace_events.next;
return t_next(m, NULL, pos);
}
@@ -303,6 +308,9 @@ s_next(struct seq_file *m, void *v, loff_t *pos)
static void *s_start(struct seq_file *m, loff_t *pos)
{
+ mutex_lock(&event_mutex);
+ if (*pos == 0)
+ m->private = ftrace_events.next;
return s_next(m, NULL, pos);
}
@@ -319,12 +327,12 @@ static int t_show(struct seq_file *m, void *v)
static void t_stop(struct seq_file *m, void *p)
{
+ mutex_unlock(&event_mutex);
}
static int
ftrace_event_seq_open(struct inode *inode, struct file *file)
{
- int ret;
const struct seq_operations *seq_ops;
if ((file->f_mode & FMODE_WRITE) &&
@@ -332,13 +340,7 @@ ftrace_event_seq_open(struct inode *inode, struct file *file)
ftrace_clear_events();
seq_ops = inode->i_private;
- ret = seq_open(file, seq_ops);
- if (!ret) {
- struct seq_file *m = file->private_data;
-
- m->private = ftrace_events.next;
- }
- return ret;
+ return seq_open(file, seq_ops);
}
static ssize_t
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index ce07b81..7ac6910 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -408,6 +408,7 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
filter->n_preds = 0;
}
+ mutex_lock(&event_mutex);
list_for_each_entry(call, &ftrace_events, list) {
if (!call->define_fields)
continue;
@@ -417,6 +418,7 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
remove_filter_string(call->filter);
}
}
+ mutex_unlock(&event_mutex);
}
static int filter_add_pred_fn(struct filter_parse_state *ps,
@@ -567,6 +569,7 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
{
struct event_filter *filter = system->filter;
struct ftrace_event_call *call;
+ int err = 0;
if (!filter->preds) {
filter->preds = kzalloc(MAX_FILTER_PRED * sizeof(pred),
@@ -584,8 +587,8 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
filter->preds[filter->n_preds] = pred;
filter->n_preds++;
+ mutex_lock(&event_mutex);
list_for_each_entry(call, &ftrace_events, list) {
- int err;
if (!call->define_fields)
continue;
@@ -597,12 +600,13 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
if (err) {
filter_free_subsystem_preds(system);
parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);
- return err;
+ break;
}
replace_filter_string(call->filter, filter_string);
}
+ mutex_unlock(&event_mutex);
- return 0;
+ return err;
}
static void parse_init(struct filter_parse_state *ps,
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [tip:tracing/core] tracing/events: fix concurrent access to ftrace_events list
2009-05-06 12:19 ` [tip:tracing/core] " tip-bot for Li Zefan
@ 2009-05-07 7:11 ` Li Zefan
2009-05-07 8:09 ` Ingo Molnar
2009-05-07 9:20 ` [tip:tracing/core] tracing/events: fix concurrent access to ftrace_events list, fix tip-bot for Li Zefan
0 siblings, 2 replies; 16+ messages in thread
From: Li Zefan @ 2009-05-07 7:11 UTC (permalink / raw)
To: mingo
Cc: hpa, linux-kernel, tzanussi, a.p.zijlstra, fweisbec, rostedt,
tglx, linux-tip-commits
tip-bot for Li Zefan wrote:
> Commit-ID: 20c8928abe70e204bd077ab6cfe23002d7788983
> Gitweb: http://git.kernel.org/tip/20c8928abe70e204bd077ab6cfe23002d7788983
> Author: Li Zefan <lizf@cn.fujitsu.com>
> AuthorDate: Wed, 6 May 2009 10:33:45 +0800
> Committer: Ingo Molnar <mingo@elte.hu>
> CommitDate: Wed, 6 May 2009 10:38:19 +0200
>
> tracing/events: fix concurrent access to ftrace_events list
>
There is a deadlock in this patch, and I sent out a v2 patch and
expected it to be applied:
http://lkml.org/lkml/2009/5/6/26
Below is a fix on top of this patch. Sorry for my carelessness.
=========
From: Li Zefan <lizf@cn.fujitsu.com>
Subject: [PATCH] tracing/events: fix deadlock on event_mutex
In filter_add_subsystem_pred() we should release event_mutex before
calling filter_free_subsystem_preds(), since both functions hold
event_mutex.
[ Impact: fix deadlock when writing invalid pred into subsystem filter ]
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
kernel/trace/trace_events_filter.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 8c62e5b..85ad6a8 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -636,14 +636,15 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
err = filter_add_pred(ps, call, pred);
if (err) {
+ mutex_unlock(&event_mutex);
filter_free_subsystem_preds(system);
parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);
- break;
+ goto out;
}
replace_filter_string(call->filter, filter_string);
}
mutex_unlock(&event_mutex);
-
+out:
return err;
}
--
1.5.4.rc3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [tip:tracing/core] tracing/events: fix concurrent access to ftrace_events list
2009-05-07 7:11 ` Li Zefan
@ 2009-05-07 8:09 ` Ingo Molnar
2009-05-07 9:20 ` [tip:tracing/core] tracing/events: fix concurrent access to ftrace_events list, fix tip-bot for Li Zefan
1 sibling, 0 replies; 16+ messages in thread
From: Ingo Molnar @ 2009-05-07 8:09 UTC (permalink / raw)
To: Li Zefan
Cc: mingo, hpa, linux-kernel, tzanussi, a.p.zijlstra, fweisbec,
rostedt, tglx, linux-tip-commits
* Li Zefan <lizf@cn.fujitsu.com> wrote:
> tip-bot for Li Zefan wrote:
> > Commit-ID: 20c8928abe70e204bd077ab6cfe23002d7788983
> > Gitweb: http://git.kernel.org/tip/20c8928abe70e204bd077ab6cfe23002d7788983
> > Author: Li Zefan <lizf@cn.fujitsu.com>
> > AuthorDate: Wed, 6 May 2009 10:33:45 +0800
> > Committer: Ingo Molnar <mingo@elte.hu>
> > CommitDate: Wed, 6 May 2009 10:38:19 +0200
> >
> > tracing/events: fix concurrent access to ftrace_events list
> >
>
> There is a deadlock in this patch, and I sent out a v2 patch and
> expected it to be applied:
> http://lkml.org/lkml/2009/5/6/26
ah, i probably already had the first one applied.
> Below is a fix on top of this patch. Sorry for my carelessness.
thanks, applied. No need to be sorry!
btw., we might want to add self-tests for filter functionality too
perhaps?
Ingo
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tip:tracing/core] tracing/events: fix concurrent access to ftrace_events list, fix
2009-05-07 7:11 ` Li Zefan
2009-05-07 8:09 ` Ingo Molnar
@ 2009-05-07 9:20 ` tip-bot for Li Zefan
1 sibling, 0 replies; 16+ messages in thread
From: tip-bot for Li Zefan @ 2009-05-07 9:20 UTC (permalink / raw)
To: linux-tip-commits; +Cc: linux-kernel, hpa, mingo, lizf, tglx, mingo
Commit-ID: d94fc523f3c35bd8013f04827e94756cbc0212f4
Gitweb: http://git.kernel.org/tip/d94fc523f3c35bd8013f04827e94756cbc0212f4
Author: Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Thu, 7 May 2009 15:11:15 +0800
Committer: Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 7 May 2009 10:07:28 +0200
tracing/events: fix concurrent access to ftrace_events list, fix
In filter_add_subsystem_pred() we should release event_mutex before
calling filter_free_subsystem_preds(), since both functions hold
event_mutex.
[ Impact: fix deadlock when writing invalid pred into subsystem filter ]
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Cc: tzanussi@gmail.com
Cc: a.p.zijlstra@chello.nl
Cc: fweisbec@gmail.com
Cc: rostedt@goodmis.org
LKML-Reference: <4A028993.7020509@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
kernel/trace/trace_events_filter.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 8c62e5b..85ad6a8 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -636,14 +636,15 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
err = filter_add_pred(ps, call, pred);
if (err) {
+ mutex_unlock(&event_mutex);
filter_free_subsystem_preds(system);
parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);
- break;
+ goto out;
}
replace_filter_string(call->filter, filter_string);
}
mutex_unlock(&event_mutex);
-
+out:
return err;
}
^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2009-05-07 9:21 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-05-06 2:32 [PATCH 1/4] tracing/events: don't say hi when loading the trace event sample Li Zefan
2009-05-06 2:32 ` [PATCH 2/4] tracing/events: make SAMPLE_TRACE_EVENTS default to n Li Zefan
2009-05-06 12:19 ` [tip:tracing/core] " tip-bot for Li Zefan
2009-05-06 2:33 ` [PATCH 3/4] tracing/events: fix memory leak when unloading module Li Zefan
2009-05-06 2:43 ` Steven Rostedt
2009-05-06 12:19 ` [tip:tracing/core] " tip-bot for Li Zefan
2009-05-06 2:33 ` [PATCH 4/4] tracing/events: fix concurrent access to ftrace_events list Li Zefan
2009-05-06 5:27 ` Frederic Weisbecker
2009-05-06 8:40 ` Ingo Molnar
2009-05-06 5:30 ` [PATCH v2] " Li Zefan
2009-05-06 12:19 ` [tip:tracing/core] " tip-bot for Li Zefan
2009-05-07 7:11 ` Li Zefan
2009-05-07 8:09 ` Ingo Molnar
2009-05-07 9:20 ` [tip:tracing/core] tracing/events: fix concurrent access to ftrace_events list, fix tip-bot for Li Zefan
2009-05-06 2:41 ` [PATCH 1/4] tracing/events: don't say hi when loading the trace event sample Steven Rostedt
2009-05-06 12:19 ` [tip:tracing/core] " tip-bot for Li Zefan
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.