Skip to contentSkip to conversation

kprobes: can cause perf to crash the system

GIT PULL New Steven Rostedt 2 participants 4 messages Oct 10, 2011 – Oct 12, 2011

Subsystems (2)
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

Ingo,

I tested and verified that the commends that Masami has shown will crash the system. These patches do fix the problem.

I also verified that the same commands will crash 2.6.39 and have included stable in the Cc of the commits.

-- Steve

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

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

Head SHA1: 44a56040a0037a845d5fa218dffde464579f0cab

Masami Hiramatsu (2):
ftrace/kprobes: Fix not to delete probes if in use perf probe: Fix to show correct error string

----

 kernel/trace/trace_kprobe.c   |   58 ++++++++++++++++++++++++++++++++++------
 tools/perf/util/probe-event.c |    4 ++-
 2 files changed, 52 insertions(+), 10 deletions(-)
author maintainer
#1 lore raw

PATCH 1/2 ftrace/kprobes: Fix not to delete probes if in use

From: Masami Hiramatsu <[email protected]>

Fix kprobe-tracer not to delete a probe if the probe is in use. In that case, delete operation will return -EBUSY.

This bug can cause a kernel panic if enabled probes are deleted during perf record.

(Add some probes on functions)
sh-4.2# perf probe --del probe:\*
sh-4.2# exit
(kernel panic)

This is originally reported on the fedora bugzilla:

https://bugzilla.redhat.com/show_bug.cgi?id=742383

I've also checked that this problem doesn't happen on tracepoints when module removing because perf event locks target module.

$ sudo ./perf record -e xfs:\* -aR sh
sh-4.2# rmmod xfs
ERROR: Module xfs is in use
sh-4.2# exit
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.203 MB perf.data (~8862 samples) ]
---
 kernel/trace/trace_kprobe.c |   58 ++++++++++++++++++++++++++++++++++++------
 1 files changed, 49 insertions(+), 9 deletions(-)
kernel/trace/trace_kprobe.c +49 -9
index 5fb3697..00d527c 100644
@@ -836,11 +836,17 @@ static void __unregister_trace_probe(struct trace_probe *tp)
 }
 
 /* Unregister a trace_probe and probe_event: call with locking probe_lock */
-static void unregister_trace_probe(struct trace_probe *tp)
+static int unregister_trace_probe(struct trace_probe *tp)
 {
+	/* Enabled event can not be unregistered */
+	if (trace_probe_is_enabled(tp))
+		return -EBUSY;
+
 	__unregister_trace_probe(tp);
 	list_del(&tp->list);
 	unregister_probe_event(tp);
+
+	return 0;
 }
 
 /* Register a trace_probe and probe_event */
@@ -854,7 +860,9 @@ static int register_trace_probe(struct trace_probe *tp)
 	/* Delete old (same name) event if exist */
 	old_tp = find_trace_probe(tp->call.name, tp->call.class->system);
 	if (old_tp) {
-		unregister_trace_probe(old_tp);
+		ret = unregister_trace_probe(old_tp);
+		if (ret < 0)
+			goto end;
 		free_trace_probe(old_tp);
 	}
 
@@ -892,6 +900,7 @@ static int trace_probe_module_callback(struct notifier_block *nb,
 	mutex_lock(&probe_lock);
 	list_for_each_entry(tp, &probe_list, list) {
 		if (trace_probe_within_module(tp, mod)) {
+			/* Don't need to check busy - this should have gone. */
 			__unregister_trace_probe(tp);
 			ret = __register_trace_probe(tp);
 			if (ret)
@@ -1205,10 +1214,11 @@ static int create_trace_probe(int argc, char **argv)
 			return -ENOENT;
 		}
 		/* delete an event */
-		unregister_trace_probe(tp);
-		free_trace_probe(tp);
+		ret = unregister_trace_probe(tp);
+		if (ret == 0)
+			free_trace_probe(tp);
 		mutex_unlock(&probe_lock);
-		return 0;
+		return ret;
 	}
 
 	if (argc < 2) {
@@ -1317,18 +1327,29 @@ error:
 	return ret;
 }
 
-static void release_all_trace_probes(void)
+static int release_all_trace_probes(void)
 {
 	struct trace_probe *tp;
+	int ret = 0;
 
 	mutex_lock(&probe_lock);
+	/* Ensure no probe is in use. */
+	list_for_each_entry(tp, &probe_list, list)
+		if (trace_probe_is_enabled(tp)) {
+			ret = -EBUSY;
+			goto end;
+		}
 	/* TODO: Use batch unregistration */
 	while (!list_empty(&probe_list)) {
 		tp = list_entry(probe_list.next, struct trace_probe, list);
 		unregister_trace_probe(tp);
 		free_trace_probe(tp);
 	}
+
+end:
 	mutex_unlock(&probe_lock);
+
+	return ret;
 }
 
 /* Probes listing interfaces */
@@ -1380,9 +1401,13 @@ static const struct seq_operations probes_seq_op = {
 
 static int probes_open(struct inode *inode, struct file *file)
 {
-	if ((file->f_mode & FMODE_WRITE) &&
-	    (file->f_flags & O_TRUNC))
-		release_all_trace_probes();
+	int ret;
+
+	if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) {
+		ret = release_all_trace_probes();
+		if (ret < 0)
+			return ret;
+	}
 
 	return seq_open(file, &probes_seq_op);
 }
@@ -2055,6 +2080,21 @@ static __init int kprobe_trace_self_tests_init(void)
 
 	ret = target(1, 2, 3, 4, 5, 6);
 
+	/* Disable trace points before removing it */
+	tp = find_trace_probe("testprobe", KPROBE_EVENT_SYSTEM);
+	if (WARN_ON_ONCE(tp == NULL)) {
+		pr_warning("error on getting test probe.\n");
+		warn++;
+	} else
+		disable_trace_probe(tp, TP_FLAG_TRACE);
+
+	tp = find_trace_probe("testprobe2", KPROBE_EVENT_SYSTEM);
+	if (WARN_ON_ONCE(tp == NULL)) {
+		pr_warning("error on getting 2nd test probe.\n");
+		warn++;
+	} else
+		disable_trace_probe(tp, TP_FLAG_TRACE);
+
 	ret = command_trace_probe("-:testprobe");
 	if (WARN_ON_ONCE(ret)) {
 		pr_warning("error on deleting a probe.\n");
Signature · 1 line
1.7.6.3
author maintainer
#2 lore raw

PATCH 2/2 perf probe: Fix to show correct error string

In reply to Steven Rostedt

From: Masami Hiramatsu <[email protected]>

Fix perf probe to show correct error string when it fails to delete an event. The write(2) returns -1 if failed, and errno stores real error number.

---
 tools/perf/util/probe-event.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
tools/perf/util/probe-event.c +3 -1
index 1c7bfa5..eb25900 100644
@@ -1956,8 +1956,10 @@ static int __del_trace_probe_event(int fd, struct str_node *ent)
 
 	pr_debug("Writing event: %s\n", buf);
 	ret = write(fd, buf, strlen(buf));
-	if (ret < 0)
+	if (ret < 0) {
+		ret = -errno;
 		goto error;
+	}
 
 	printf("Remove event: %s\n", ent->s);
 	return 0;
Signature · 1 line
1.7.6.3
#3 lore raw
In reply to Steven Rostedt
Quoted 19 lines · Steven Rostedt

Ingo,

I tested and verified that the commends that Masami has shown will crash the system. These patches do fix the problem.

I also verified that the same commands will crash 2.6.39 and have included stable in the Cc of the commits.

-- Steve

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

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

Head SHA1: 44a56040a0037a845d5fa218dffde464579f0cab

Quoted 8 lines

Masami Hiramatsu (2):
ftrace/kprobes: Fix not to delete probes if in use perf probe: Fix to show correct error string

----

 kernel/trace/trace_kprobe.c   |   58 ++++++++++++++++++++++++++++++++++------
 tools/perf/util/probe-event.c |    4 ++-
 2 files changed, 52 insertions(+), 10 deletions(-)

Pulled, thanks a lot guys!

Ingo