Re: Re: [RFC][PATCH 3/4] tracing/kprobes: Fail to unregister if probe event files are open

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

(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