LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/4] [BUGFIX] ftrace/kprobes: Fix not to delete probes if in use
@ 2011-10-04 10:44 Masami Hiramatsu
  2011-10-04 10:44 ` [PATCH 2/4] [TRIVIAL] ftrace/kprobes: Fix an warning typo Masami Hiramatsu
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Masami Hiramatsu @ 2011-10-04 10:44 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ingo Molnar
  Cc: linux-kernel, yrl.pp-manager.tt, Masami Hiramatsu,
	Steven Rostedt, Frederic Weisbecker, Ingo Molnar,
	Frank Ch. Eigler

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

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Frank Ch. Eigler <fche@redhat.com>
---

 kernel/trace/trace_kprobe.c |   58 ++++++++++++++++++++++++++++++++++++-------
 1 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 5fb3697..00d527c 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -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");


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/4] [TRIVIAL] ftrace/kprobes: Fix an warning typo
  2011-10-04 10:44 [PATCH 1/4] [BUGFIX] ftrace/kprobes: Fix not to delete probes if in use Masami Hiramatsu
@ 2011-10-04 10:44 ` Masami Hiramatsu
  2011-10-04 15:58   ` Valdis.Kletnieks
  2011-10-04 10:44 ` [PATCH 3/4] [TRIVIAL] perftools: Fix a typo of command name as trace-cmd Masami Hiramatsu
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Masami Hiramatsu @ 2011-10-04 10:44 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ingo Molnar
  Cc: linux-kernel, yrl.pp-manager.tt, Masami Hiramatsu,
	Steven Rostedt, Frederic Weisbecker, Ingo Molnar

Add a space between words.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
---

 kernel/trace/trace_kprobe.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 00d527c..5763546 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -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) {


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/4] [TRIVIAL] perftools: Fix a typo of command name as trace-cmd
  2011-10-04 10:44 [PATCH 1/4] [BUGFIX] ftrace/kprobes: Fix not to delete probes if in use Masami Hiramatsu
  2011-10-04 10:44 ` [PATCH 2/4] [TRIVIAL] ftrace/kprobes: Fix an warning typo Masami Hiramatsu
@ 2011-10-04 10:44 ` Masami Hiramatsu
  2011-10-04 10:45 ` [PATCH 4/4] [BUGFIX] perf probe: Fix to show correct error string Masami Hiramatsu
  2011-10-08  5:00 ` [PATCH 1/4] [BUGFIX] ftrace/kprobes: Fix not to delete probes if in use Steven Rostedt
  3 siblings, 0 replies; 9+ messages in thread
From: Masami Hiramatsu @ 2011-10-04 10:44 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ingo Molnar
  Cc: linux-kernel, yrl.pp-manager.tt, Masami Hiramatsu,
	Peter Zijlstra, Paul Mackerras, Ingo Molnar,
	Arnaldo Carvalho de Melo, Steven Rostedt

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

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Steven Rostedt <rostedt@goodmis.org>
---

 tools/perf/util/trace-event-info.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 3403f81..2ed5ab8 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -80,7 +80,7 @@ static void die(const char *fmt, ...)
 	int ret = errno;
 
 	if (errno)
-		perror("trace-cmd");
+		perror("perf");
 	else
 		ret = -1;
 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 4/4] [BUGFIX] perf probe: Fix to show correct error string
  2011-10-04 10:44 [PATCH 1/4] [BUGFIX] ftrace/kprobes: Fix not to delete probes if in use Masami Hiramatsu
  2011-10-04 10:44 ` [PATCH 2/4] [TRIVIAL] ftrace/kprobes: Fix an warning typo Masami Hiramatsu
  2011-10-04 10:44 ` [PATCH 3/4] [TRIVIAL] perftools: Fix a typo of command name as trace-cmd Masami Hiramatsu
@ 2011-10-04 10:45 ` Masami Hiramatsu
  2011-10-08  5:00 ` [PATCH 1/4] [BUGFIX] ftrace/kprobes: Fix not to delete probes if in use Steven Rostedt
  3 siblings, 0 replies; 9+ messages in thread
From: Masami Hiramatsu @ 2011-10-04 10:45 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ingo Molnar
  Cc: linux-kernel, yrl.pp-manager.tt, Masami Hiramatsu,
	Peter Zijlstra, Paul Mackerras, Ingo Molnar,
	Arnaldo Carvalho de Melo, Steven Rostedt

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.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Steven Rostedt <rostedt@goodmis.org>
---

 tools/perf/util/probe-event.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 1c7bfa5..eb25900 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -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;


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/4] [TRIVIAL] ftrace/kprobes: Fix an warning typo
  2011-10-04 10:44 ` [PATCH 2/4] [TRIVIAL] ftrace/kprobes: Fix an warning typo Masami Hiramatsu
@ 2011-10-04 15:58   ` Valdis.Kletnieks
  2011-10-05  4:08     ` Masami Hiramatsu
  0 siblings, 1 reply; 9+ messages in thread
From: Valdis.Kletnieks @ 2011-10-04 15:58 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, linux-kernel,
	yrl.pp-manager.tt, Steven Rostedt, Frederic Weisbecker,
	Ingo Molnar


[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

On Tue, 04 Oct 2011 19:44:49 +0900, Masami Hiramatsu said:
> Add a space between words.

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


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/4] [TRIVIAL] ftrace/kprobes: Fix an warning typo
  2011-10-04 15:58   ` Valdis.Kletnieks
@ 2011-10-05  4:08     ` Masami Hiramatsu
  0 siblings, 0 replies; 9+ messages in thread
From: Masami Hiramatsu @ 2011-10-05  4:08 UTC (permalink / raw)
  To: Valdis.Kletnieks
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, linux-kernel,
	yrl.pp-manager.tt, Steven Rostedt, Frederic Weisbecker,
	Ingo Molnar

(2011/10/05 0:58), Valdis.Kletnieks@vt.edu wrote:
> On Tue, 04 Oct 2011 19:44:49 +0900, Masami Hiramatsu said:
>> Add a space between words.
> 
>>  		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,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] [BUGFIX] ftrace/kprobes: Fix not to delete probes if in use
  2011-10-04 10:44 [PATCH 1/4] [BUGFIX] ftrace/kprobes: Fix not to delete probes if in use Masami Hiramatsu
                   ` (2 preceding siblings ...)
  2011-10-04 10:45 ` [PATCH 4/4] [BUGFIX] perf probe: Fix to show correct error string Masami Hiramatsu
@ 2011-10-08  5:00 ` Steven Rostedt
  2011-10-10 14:14   ` Masami Hiramatsu
  3 siblings, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2011-10-08  5:00 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, linux-kernel,
	yrl.pp-manager.tt, Frederic Weisbecker, Ingo Molnar,
	Frank Ch. Eigler

On Tue, 2011-10-04 at 19:44 +0900, Masami Hiramatsu wrote:
> 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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] [BUGFIX] ftrace/kprobes: Fix not to delete probes if in use
  2011-10-08  5:00 ` [PATCH 1/4] [BUGFIX] ftrace/kprobes: Fix not to delete probes if in use Steven Rostedt
@ 2011-10-10 14:14   ` Masami Hiramatsu
  2011-10-10 19:11     ` Steven Rostedt
  0 siblings, 1 reply; 9+ messages in thread
From: Masami Hiramatsu @ 2011-10-10 14:14 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, linux-kernel,
	yrl.pp-manager.tt, Frederic Weisbecker, Ingo Molnar,
	Frank Ch. Eigler

(2011/10/08 14:00), Steven Rostedt wrote:
> On Tue, 2011-10-04 at 19:44 +0900, Masami Hiramatsu wrote:
>> 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.

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

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

> 
> Thanks!
> 
> -- Steve
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] [BUGFIX] ftrace/kprobes: Fix not to delete probes if in use
  2011-10-10 14:14   ` Masami Hiramatsu
@ 2011-10-10 19:11     ` Steven Rostedt
  0 siblings, 0 replies; 9+ messages in thread
From: Steven Rostedt @ 2011-10-10 19:11 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, linux-kernel,
	yrl.pp-manager.tt, Frederic Weisbecker, Ingo Molnar,
	Frank Ch. Eigler

On Mon, 2011-10-10 at 23:14 +0900, Masami Hiramatsu wrote:

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-10-04 10:44 [PATCH 1/4] [BUGFIX] ftrace/kprobes: Fix not to delete probes if in use Masami Hiramatsu
2011-10-04 10:44 ` [PATCH 2/4] [TRIVIAL] ftrace/kprobes: Fix an warning typo Masami Hiramatsu
2011-10-04 15:58   ` Valdis.Kletnieks
2011-10-05  4:08     ` Masami Hiramatsu
2011-10-04 10:44 ` [PATCH 3/4] [TRIVIAL] perftools: Fix a typo of command name as trace-cmd Masami Hiramatsu
2011-10-04 10:45 ` [PATCH 4/4] [BUGFIX] perf probe: Fix to show correct error string Masami Hiramatsu
2011-10-08  5:00 ` [PATCH 1/4] [BUGFIX] ftrace/kprobes: Fix not to delete probes if in use Steven Rostedt
2011-10-10 14:14   ` Masami Hiramatsu
2011-10-10 19:11     ` Steven Rostedt

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git