[PATCH 1/4] tracing/events: don't say hi when loading the trace event sample

[PATCH 1/4] tracing/events: don't say hi when loading the trace event sample

[Li Zefan]

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

[PATCH 2/4] tracing/events: make SAMPLE_TRACE_EVENTS default to n

[Li Zefan]

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

[tip:tracing/core] tracing/events: make SAMPLE_TRACE_EVENTS default to n

[tip-bot for Li Zefan]

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.
 

[PATCH 3/4] tracing/events: fix memory leak when unloading module

[Li Zefan]

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

Re: [PATCH 3/4] tracing/events: fix memory leak when unloading module

[Steven Rostedt]

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

[tip:tracing/core] tracing/events: fix memory leak when unloading module

[tip-bot for Li Zefan]

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

[PATCH 4/4] tracing/events: fix concurrent access to ftrace_events list

[Li Zefan]

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

Re: [PATCH 4/4] tracing/events: fix concurrent access to ftrace_events list

[Frederic Weisbecker]

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

Re: [PATCH 4/4] tracing/events: fix concurrent access to ftrace_events list

[Ingo Molnar]

* 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

[PATCH v2] tracing/events: fix concurrent access to ftrace_events list

[Li Zefan]

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

[tip:tracing/core] tracing/events: fix concurrent access to ftrace_events list

[tip-bot for Li Zefan]

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,

Re: [tip:tracing/core] tracing/events: fix concurrent access to ftrace_events list

[Li Zefan]

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

Re: [tip:tracing/core] tracing/events: fix concurrent access to ftrace_events list

[Ingo Molnar]

* 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

[tip:tracing/core] tracing/events: fix concurrent access to ftrace_events list, fix

[tip-bot for Li Zefan]

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

Re: [PATCH 1/4] tracing/events: don't say hi when loading the trace event sample

[Steven Rostedt]

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

[tip:tracing/core] tracing/events: don't say hi when loading the trace event sample

[tip-bot for Li Zefan]

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)