Skip to contentSkip to conversation

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

PATCH In review Masami Hiramatsu 3 participants 9 messages Oct 4, 2011 – Oct 10, 2011

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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

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)

# perf record -e probe:\* -aR sh
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) ]
Cc Steven Rostedt <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Frank Ch. Eigler <[email protected]>

---

 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");
#1 lore raw

PATCH 2/4 ftrace/kprobes: Fix an warning typo

Add a space between words.

Cc Steven Rostedt <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ingo Molnar <[email protected]>

---

 kernel/trace/trace_kprobe.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/trace/trace_kprobe.c +1 -1
index 00d527c..5763546 100644
@@ -806,7 +806,7 @@ static int __register_trace_probe(struct trace_probe *tp)
 		pr_warning("Could not insert probe at %s+%lu: %d\n",
 			   trace_probe_symbol(tp), trace_probe_offset(tp), ret);
 		if (ret == -ENOENT && trace_probe_is_on_module(tp)) {
-			pr_warning("This probe might be able to register after"
+			pr_warning("This probe might be able to register after "
 				   "target module is loaded. Continue.\n");
 			ret = 0;
 		} else if (ret == -EILSEQ) {
#2 lore raw

Re: [PATCH 2/4] [TRIVIAL] ftrace/kprobes: Fix an warning typo

kernel/trace/trace_kprobe.c static int __register_trace_probe(struct trace_probe *tp) · line 810

Quoted 6 lines · Masami Hiramatsu

Add a space between words.

kernel/trace/trace_kprobe.c +1 -1
 		if (ret == -ENOENT && trace_probe_is_on_module(tp)) {
-			pr_warning("This probe might be able to register after"
+			pr_warning("This probe might be able to register after "
 				   "target module is loaded. Continue.\n");

Better would be to make it all one line. Keeping a string together trumps the 80-column rule.

#3 lore raw

Re: [PATCH 2/4] [TRIVIAL] ftrace/kprobes: Fix an warning typo

Quoted 11 lines · (2011/10/05 0:58), [email protected] wrote:

kernel/trace/trace_kprobe.c static int __register_trace_probe(struct trace_probe *tp) · line 810

Quoted 6 lines · Masami Hiramatsu

Add a space between words.

kernel/trace/trace_kprobe.c +1 -1
 		if (ret == -ENOENT && trace_probe_is_on_module(tp)) {
-			pr_warning("This probe might be able to register after"
+			pr_warning("This probe might be able to register after "
 				   "target module is loaded. Continue.\n");

Better would be to make it all one line. Keeping a string together trumps the 80-column rule.

Agreed :) I'd like to check whether other messages should be fixed too.

Thank you,

Signature · 4 lines
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
#4 lore raw

PATCH 3/4 perftools: Fix a typo of command name as trace-cmd

In reply to Masami Hiramatsu

Fix a typo which may be introduced when original
code has been copied from trace-cmd.

Cc Peter Zijlstra <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Steven Rostedt <[email protected]>

---

 tools/perf/util/trace-event-info.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
tools/perf/util/trace-event-info.c +1 -1
index 3403f81..2ed5ab8 100644
@@ -80,7 +80,7 @@ static void die(const char *fmt, ...)
 	int ret = errno;
 
 	if (errno)
-		perror("trace-cmd");
+		perror("perf");
 	else
 		ret = -1;
 
#5 lore raw

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

In reply to Masami Hiramatsu

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.

Cc Peter Zijlstra <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Steven Rostedt <[email protected]>

---

 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;
maintainer
#6 lore raw
In reply to Masami Hiramatsu
Quoted 11 lines · Masami Hiramatsu

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)

# perf record -e probe:\* -aR sh
sh-4.2# perf probe --del probe:\*
sh-4.2# exit
(kernel panic)

Hi Masami,

I was able to reproduce the panic. I'm currently running patch 1 and 4 through my standard tests before pushing this out as urgent.

Does this exist in previous kernels? If so, I'll also add the stable tag to it.

Could you resend the trivial patches when you make your necessary corrections. They are not important as these are and I'll add them to a separate queue.

Thanks!

-- Steve

#7 lore raw
Quoted 14 lines · Steven Rostedt
Quoted 11 lines · Masami Hiramatsu

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)

# perf record -e probe:\* -aR sh
sh-4.2# perf probe --del probe:\*
sh-4.2# exit
(kernel panic)
Quoted 7 lines

Hi Masami,

I was able to reproduce the panic. I'm currently running patch 1 and 4 through my standard tests before pushing this out as urgent.

Does this exist in previous kernels? If so, I'll also add the stable tag to it.

I guess so, since the perf's tracepoint handler locks target module while recording, it is required for perf not to remove undergo events.

Quoted 3 lines

Could you resend the trivial patches when you make your necessary corrections. They are not important as these are and I'll add them to a separate queue.

Right, that's not a matter. The first patch should be merged soon.

Thank you,

Quoted 11 lines

Thanks!

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 4 lines
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
maintainer
#8 lore raw
Quoted 5 lines · Masami Hiramatsu
Quoted 2 lines

Does this exist in previous kernels? If so, I'll also add the stable tag to it.

I guess so, since the perf's tracepoint handler locks target module while recording, it is required for perf not to remove undergo events.

I was able to trigger this bug in 2.6.39 as well. I'll add the stable tag to it and post it.

Thanks!

-- Steve