ftrace/kprobes: Fix not to delete probes if in use
Subsystems (2)
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) ]
---
kernel/trace/trace_kprobe.c | 58 ++++++++++++++++++++++++++++++++++++------- 1 files changed, 49 insertions(+), 9 deletions(-)
kernel/trace/trace_kprobe.c +49 -9
@@ -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" );
PATCH 2/4 ftrace/kprobes: Fix an warning typo
Add a space between words.
---
kernel/trace/trace_kprobe.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/trace/trace_kprobe.c +1 -1
@@ -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 ) {
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
-1if (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.
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
-1if (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]
PATCH 3/4 perftools: Fix a typo of command name as trace-cmd
In reply to Masami HiramatsuFix a typo which may be introduced when original
code has been copied from trace-cmd.
---
tools/perf/util/trace-event-info.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
tools/perf/util/trace-event-info.c +1 -1
@@ -80,7 +80,7 @@ static void die(const char *fmt, ...)int ret = errno;if (errno) -perror ("trace-cmd" ); +perror ("perf" );else ret =-1 ;
PATCH 4/4 perf probe: Fix to show correct error string
In reply to Masami HiramatsuFix 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
@@ -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 ;
Steven Rostedt
maintainerQuoted 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
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]
Steven Rostedt
maintainerQuoted 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