lkml.org 
[lkml]   [2013]   [Jul]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: Re: [RFC][PATCH 3/4] tracing/kprobes: Fail to unregister if probe event files are open
    (2013/08/01 11:50), Steven Rostedt wrote:
    > Here's the new change log, but the same patch. Does this sound ok to you
    > guys?

    Great! This looks good for me. ;)

    Thank you very much!

    >
    > -- Steve
    >
    >>From 40c32592668b727cbfcf7b1c0567f581bd62a5e4 Mon Sep 17 00:00:00 2001
    > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
    > Date: Wed, 3 Jul 2013 23:33:50 -0400
    > Subject: [PATCH] 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+ #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>
    > ---
    > kernel/trace/trace_kprobe.c | 21 +++++++++++++++------
    > 1 file changed, 15 insertions(+), 6 deletions(-)
    >
    > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
    > index 3811487..243f683 100644
    > --- a/kernel/trace/trace_kprobe.c
    > +++ b/kernel/trace/trace_kprobe.c
    > @@ -95,7 +95,7 @@ static __kprobes bool trace_probe_is_on_module(struct trace_probe *tp)
    > }
    >
    > static int register_probe_event(struct trace_probe *tp);
    > -static void unregister_probe_event(struct trace_probe *tp);
    > +static int unregister_probe_event(struct trace_probe *tp);
    >
    > static DEFINE_MUTEX(probe_lock);
    > static LIST_HEAD(probe_list);
    > @@ -351,9 +351,12 @@ static int unregister_trace_probe(struct trace_probe *tp)
    > if (trace_probe_is_enabled(tp))
    > return -EBUSY;
    >
    > + /* Will fail if probe is being used by ftrace or perf */
    > + if (unregister_probe_event(tp))
    > + return -EBUSY;
    > +
    > __unregister_trace_probe(tp);
    > list_del(&tp->list);
    > - unregister_probe_event(tp);
    >
    > return 0;
    > }
    > @@ -632,7 +635,9 @@ static int release_all_trace_probes(void)
    > /* TODO: Use batch unregistration */
    > while (!list_empty(&probe_list)) {
    > tp = list_entry(probe_list.next, struct trace_probe, list);
    > - unregister_trace_probe(tp);
    > + ret = unregister_trace_probe(tp);
    > + if (ret)
    > + goto end;
    > free_trace_probe(tp);
    > }
    >
    > @@ -1247,11 +1252,15 @@ static int register_probe_event(struct trace_probe *tp)
    > return ret;
    > }
    >
    > -static void unregister_probe_event(struct trace_probe *tp)
    > +static int unregister_probe_event(struct trace_probe *tp)
    > {
    > + int ret;
    > +
    > /* tp->event is unregistered in trace_remove_event_call() */
    > - trace_remove_event_call(&tp->call);
    > - kfree(tp->call.print_fmt);
    > + ret = trace_remove_event_call(&tp->call);
    > + if (!ret)
    > + kfree(tp->call.print_fmt);
    > + return ret;
    > }
    >
    > /* Make a debugfs interface for controlling probe points */
    >


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




    \
     
     \ /
      Last update: 2013-08-01 06:01    [W:2.907 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site