Skip to content
This repository was archived by the owner on Dec 20, 2023. It is now read-only.

Commit 40c3259

Browse files
committed
tracing/kprobes: Fail to unregister if probe event files are in use
When a probe is being removed, it cleans up the event files that correspond to the probe. But there is a race between writing to one of these files and deleting the probe. This is especially true for the "enable" file. CPU 0 CPU 1 ----- ----- fd = open("enable",O_WRONLY); probes_open() release_all_trace_probes() unregister_trace_probe() if (trace_probe_is_enabled(tp)) return -EBUSY write(fd, "1", 1) __ftrace_set_clr_event() call->class->reg() (kprobe_register) enable_trace_probe(tp) __unregister_trace_probe(tp); list_del(&tp->list) unregister_probe_event(tp) <-- fails! free_trace_probe(tp) write(fd, "0", 1) __ftrace_set_clr_event() call->class->unreg (kprobe_register) disable_trace_probe(tp) <-- BOOM! A test program was written that used two threads to simulate the above scenario adding a nanosleep() interval to change the timings and after several thousand runs, it was able to trigger this bug and crash: BUG: unable to handle kernel paging request at 00000005000000f9 IP: [<ffffffff810dee70>] probes_open+0x3b/0xa7 PGD 7808a067 PUD 0 Oops: 0000 [#1] PREEMPT SMP Dumping ftrace buffer: --------------------------------- Modules linked in: ipt_MASQUERADE sunrpc ip6t_REJECT nf_conntrack_ipv6 CPU: 1 PID: 2070 Comm: test-kprobe-rem Not tainted 3.11.0-rc3-test+ torvalds#47 Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./To be filled by O.E.M., BIOS SDBLI944.86P 05/08/2007 task: ffff880077756440 ti: ffff880076e52000 task.ti: ffff880076e52000 RIP: 0010:[<ffffffff810dee70>] [<ffffffff810dee70>] probes_open+0x3b/0xa7 RSP: 0018:ffff880076e53c38 EFLAGS: 00010203 RAX: 0000000500000001 RBX: ffff88007844f440 RCX: 0000000000000003 RDX: 0000000000000003 RSI: 0000000000000003 RDI: ffff880076e52000 RBP: ffff880076e53c58 R08: ffff880076e53bd8 R09: 0000000000000000 R10: ffff880077756440 R11: 0000000000000006 R12: ffffffff810dee35 R13: ffff880079250418 R14: 0000000000000000 R15: ffff88007844f450 FS: 00007f87a276f700(0000) GS:ffff88007d480000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b CR2: 00000005000000f9 CR3: 0000000077262000 CR4: 00000000000007e0 Stack: ffff880076e53c58 ffffffff81219ea0 ffff88007844f440 ffffffff810dee35 ffff880076e53ca8 ffffffff81130f78 ffff8800772986c0 ffff8800796f93a0 ffffffff81d1b5d8 ffff880076e53e04 0000000000000000 ffff88007844f440 Call Trace: [<ffffffff81219ea0>] ? security_file_open+0x2c/0x30 [<ffffffff810dee35>] ? unregister_trace_probe+0x4b/0x4b [<ffffffff81130f78>] do_dentry_open+0x162/0x226 [<ffffffff81131186>] finish_open+0x46/0x54 [<ffffffff8113f30b>] do_last+0x7f6/0x996 [<ffffffff8113cc6f>] ? inode_permission+0x42/0x44 [<ffffffff8113f6dd>] path_openat+0x232/0x496 [<ffffffff8113fc30>] do_filp_open+0x3a/0x8a [<ffffffff8114ab32>] ? __alloc_fd+0x168/0x17a [<ffffffff81131f4e>] do_sys_open+0x70/0x102 [<ffffffff8108f06e>] ? trace_hardirqs_on_caller+0x160/0x197 [<ffffffff81131ffe>] SyS_open+0x1e/0x20 [<ffffffff81522742>] system_call_fastpath+0x16/0x1b Code: e5 41 54 53 48 89 f3 48 83 ec 10 48 23 56 78 48 39 c2 75 6c 31 f6 48 c7 RIP [<ffffffff810dee70>] probes_open+0x3b/0xa7 RSP <ffff880076e53c38> CR2: 00000005000000f9 ---[ end trace 35f17d68fc569897 ]--- The unregister_trace_probe() must be done first, and if it fails it must fail the removal of the kprobe. Several changes have already been made by Oleg Nesterov and Masami Hiramatsu to allow moving the unregister_probe_event() before the removal of the probe and exit the function if it fails. This prevents the tp structure from being used after it is freed. Link: http://lkml.kernel.org/r/20130704034038.819592356@goodmis.org Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
1 parent 2ba6403 commit 40c3259

File tree

1 file changed

+15
-6
lines changed

1 file changed

+15
-6
lines changed

kernel/trace/trace_kprobe.c

+15-6
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ static __kprobes bool trace_probe_is_on_module(struct trace_probe *tp)
9595
}
9696

9797
static int register_probe_event(struct trace_probe *tp);
98-
static void unregister_probe_event(struct trace_probe *tp);
98+
static int unregister_probe_event(struct trace_probe *tp);
9999

100100
static DEFINE_MUTEX(probe_lock);
101101
static LIST_HEAD(probe_list);
@@ -351,9 +351,12 @@ static int unregister_trace_probe(struct trace_probe *tp)
351351
if (trace_probe_is_enabled(tp))
352352
return -EBUSY;
353353

354+
/* Will fail if probe is being used by ftrace or perf */
355+
if (unregister_probe_event(tp))
356+
return -EBUSY;
357+
354358
__unregister_trace_probe(tp);
355359
list_del(&tp->list);
356-
unregister_probe_event(tp);
357360

358361
return 0;
359362
}
@@ -632,7 +635,9 @@ static int release_all_trace_probes(void)
632635
/* TODO: Use batch unregistration */
633636
while (!list_empty(&probe_list)) {
634637
tp = list_entry(probe_list.next, struct trace_probe, list);
635-
unregister_trace_probe(tp);
638+
ret = unregister_trace_probe(tp);
639+
if (ret)
640+
goto end;
636641
free_trace_probe(tp);
637642
}
638643

@@ -1247,11 +1252,15 @@ static int register_probe_event(struct trace_probe *tp)
12471252
return ret;
12481253
}
12491254

1250-
static void unregister_probe_event(struct trace_probe *tp)
1255+
static int unregister_probe_event(struct trace_probe *tp)
12511256
{
1257+
int ret;
1258+
12521259
/* tp->event is unregistered in trace_remove_event_call() */
1253-
trace_remove_event_call(&tp->call);
1254-
kfree(tp->call.print_fmt);
1260+
ret = trace_remove_event_call(&tp->call);
1261+
if (!ret)
1262+
kfree(tp->call.print_fmt);
1263+
return ret;
12551264
}
12561265

12571266
/* Make a debugfs interface for controlling probe points */

0 commit comments

Comments
 (0)