tracing: Add refcount for system filter and enable

GIT PULL New Steven Rostedt 2 participants 5 messages Jul 6, 2011 – Jul 7, 2011 Tracing

lore

Share & tools

lore

Steven Rostedt

author maintainer

2011-07-06 21:20 UTC #0 lore raw

Ingo,

Please pull the latest tip/perf/urgent tree, which can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/perf/urgent

Head SHA1: 57f124ca21d216fca3ba3995235c15855f9b9980

Steven Rostedt (2):
tracing: Add ref_count to event systems for freeing
tracing: Have enable system events use the subsystem_open routine

----

 kernel/trace/trace.h               |    1 +
 kernel/trace/trace_events.c        |  113 ++++++++++++++++++++++++++++++------
 kernel/trace/trace_events_filter.c |    6 ++
 3 files changed, 102 insertions(+), 18 deletions(-)

Steven Rostedt

author maintainer

2011-07-06 21:20 UTC #1 lore raw

PATCH 1/2 tracing: Add ref_count to event systems for freeing

From: Steven Rostedt <[email protected]>

The system is freed when its nr_events is set to zero. This happens when a module created an event system and then later the module is removed. Modules may share systems, so the system is allocated when it is created and freed when the modules are unloaded and all the events under the system are removed (nr_events set to zero).

The problem arises when a task opened the "filter" file for the system. If the module is unloaded and it removed the last event for that system, the system structure is freed. If the task that opened the filter file accesses the "filter" file after the system has been freed, the system will access an invalid pointer.

By adding a ref_count, and using it to keep track of what is using the event system, we can free it after all users are finished with the event system.

Cc <[email protected]>

Reported-by Johannes Berg <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/trace.h               |    1 +
 kernel/trace/trace_events.c        |   86 +++++++++++++++++++++++++++++++-----
 kernel/trace/trace_events_filter.c |    6 +++
 3 files changed, 82 insertions(+), 11 deletions(-)

kernel/trace/trace.h +1 -0

index 229f859..f807407 100644
@@ -677,6 +677,7 @@ struct event_subsystem {
 	struct dentry		*entry;
 	struct event_filter	*filter;
 	int			nr_events;
+	int			ref_count;
 };
 
 #define FILTER_PRED_INVALID	((unsigned short)-1)

kernel/trace/trace_events.c +75 ~~-11~~

133 lines not on this page · raw message

kernel/trace/trace_events_filter.c +6 -0

index 8008ddc..256764e 100644
@@ -1886,6 +1886,12 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
 
 	mutex_lock(&event_mutex);
 
+	/* Make sure the system still has events */
+	if (!system->nr_events) {
+		err = -ENODEV;
+		goto out_unlock;
+	}
+
 	if (!strcmp(strstrip(filter_string), "0")) {
 		filter_free_subsystem_preds(system);
 		remove_filter_string(system->filter);

Signature · 1 line

1.7.5.4

Steven Rostedt

author maintainer

2011-07-06 21:21 UTC #2 lore raw

PATCH 2/2 tracing: Have enable system events use the subsystem_open routine

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

As the system name can be freed when a module is removed, the reference to the system name that is passed to the enable routines to enable events at a system level can point to arbitrary memory. This value is only read, but it will just read garbage.

Change the system event enabling to use the subsystem_open routines like the system filter routines do.

Cc <[email protected]>

Reported-by Johannes Berg <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

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

kernel/trace/trace_events.c +22 -9

index ffc5b28..3e2a7c9 100644
@@ -557,7 +557,7 @@ system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
 		   loff_t *ppos)
 {
 	const char set_to_char[4] = { '?', '0', '1', 'X' };
-	const char *system = filp->private_data;
+	struct event_subsystem *system = filp->private_data;
 	struct ftrace_event_call *call;
 	char buf[2];
 	int set = 0;

@@ -568,7 +568,7 @@ system_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
 		if (!call->name || !call->class || !call->class->reg)
 			continue;
 
-		if (system && strcmp(call->class->system, system) != 0)
+		if (system && strcmp(call->class->system, system->name) != 0)
 			continue;
 
 		/*

@@ -598,7 +598,8 @@ static ssize_t
 system_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
 		    loff_t *ppos)
 {
-	const char *system = filp->private_data;
+	struct event_subsystem *system = filp->private_data;
+	const char *name = NULL;
 	unsigned long val;
 	char buf[64];
 	ssize_t ret;

@@ -622,7 +623,14 @@ system_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	if (val != 0 && val != 1)
 		return -EINVAL;
 
-	ret = __ftrace_set_clr_event(NULL, system, NULL, val);
+	/*
+	 * Opening of "enable" adds a ref count to system,
+	 * so the name is safe to use.
+	 */
+	if (system)
+		name = system->name;
+
+	ret = __ftrace_set_clr_event(NULL, name, NULL, val);
 	if (ret)
 		goto out;

@@ -862,6 +870,9 @@ static int subsystem_open(struct inode *inode, struct file *filp)
 	struct event_subsystem *system = NULL;
 	int ret;
 
+	if (!inode->i_private)
+		goto skip_search;
+
 	/* Make sure the system still exists */
 	mutex_lock(&event_mutex);
 	list_for_each_entry(system, &event_subsystems, list) {

@@ -880,8 +891,9 @@ static int subsystem_open(struct inode *inode, struct file *filp)
 	if (system != inode->i_private)
 		return -ENODEV;
 
+ skip_search:
 	ret = tracing_open_generic(inode, filp);
-	if (ret < 0)
+	if (ret < 0 && system)
 		put_system(system);
 
 	return ret;

@@ -891,7 +903,8 @@ static int subsystem_release(struct inode *inode, struct file *file)
 {
 	struct event_subsystem *system = inode->i_private;
 
-	put_system(system);
+	if (system)
+		put_system(system);
 
 	return 0;
 }

@@ -1041,10 +1054,11 @@ static const struct file_operations ftrace_subsystem_filter_fops = {
 };
 
 static const struct file_operations ftrace_system_enable_fops = {
-	.open = tracing_open_generic,
+	.open = subsystem_open,
 	.read = system_enable_read,
 	.write = system_enable_write,
 	.llseek = default_llseek,
+	.release = subsystem_release,
 };
 
 static const struct file_operations ftrace_show_header_fops = {

@@ -1133,8 +1147,7 @@ event_subsystem_dir(const char *name, struct dentry *d_events)
 			   "'%s/filter' entry\n", name);
 	}
 
-	trace_create_file("enable", 0644, system->entry,
-			  (void *)system->name,
+	trace_create_file("enable", 0644, system->entry, system,
 			  &ftrace_system_enable_fops);
 
 	return system->entry;

Signature · 1 line

1.7.5.4

Ingo Molnar

2011-07-07 14:15 UTC #3 lore raw

In reply to Steven Rostedt

Quoted 15 lines · Steven Rostedt

Ingo,
Please pull the latest tip/perf/urgent tree, which can be found at:
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/perf/urgent
Head SHA1: 57f124ca21d216fca3ba3995235c15855f9b9980
Steven Rostedt (2):
tracing: Add ref_count to event systems for freeing
tracing: Have enable system events use the subsystem_open routine

Quoted 5 lines

----

 kernel/trace/trace.h               |    1 +
 kernel/trace/trace_events.c        |  113 ++++++++++++++++++++++++++++++------
 kernel/trace/trace_events_filter.c |    6 ++
 3 files changed, 102 insertions(+), 18 deletions(-)

Hm, i'm wondering, when was this bug introduced?

Thanks,

Ingo

Steven Rostedt

author maintainer

2011-07-07 14:23 UTC #4 lore raw

Quoted 17 lines · Ingo Molnar

Quoted 15 lines · Steven Rostedt
Ingo,
Please pull the latest tip/perf/urgent tree, which can be found at:
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/perf/urgent
Head SHA1: 57f124ca21d216fca3ba3995235c15855f9b9980
Steven Rostedt (2):
tracing: Add ref_count to event systems for freeing
tracing: Have enable system events use the subsystem_open routine

Quoted 8 lines

Quoted 5 lines

----

 kernel/trace/trace.h               |    1 +
 kernel/trace/trace_events.c        |  113 ++++++++++++++++++++++++++++++------
 kernel/trace/trace_events_filter.c |    6 ++
 3 files changed, 102 insertions(+), 18 deletions(-)

Hm, i'm wondering, when was this bug introduced?

It was introduced a while ago, I believe by this commit:

commit dc82ec98a4727fd51b77e92d05fe7d2db3dcc11c
Date: Thu Jul 9 16:22:22 2009 +0800
tracing/filter: Remove empty subsystem and its directory

-- Steve