Re: [PATCH 1/7] kprobes: Disable Kprobe when ftrace arming fails

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

(2015/02/27 1:13), Petr Mladek wrote:
> arm_kprobe_ftrace() could fail, especially after introducing ftrace IPMODIFY
> flag and LifePatching. But this situation is not properly handled.
> This patch adds the most important changes.

Hmm, as you know, I actually working on it to drop IPMODIFY from kprobes except jprobe.
however, yes, that is not enough. We might better set DISABLED flag as
this does.

> First, it does not make sense to register "kprobe_ftrace_ops" if the filter was
> not set.
> 
> Second, we should remove the filter if the registration of "kprobe_ftrace_ops"
> fails. The failure might be caused by conflict between the Kprobe and
> a life patch via the IPMODIFY flag. If we remove the filter, we will allow
> to register "kprobe_ftrace_ops" for another non-conflicting Kprobe later.
> 
> Third, we need to make sure that "kprobe_ftrace_enabled" is incremented only
> when "kprobe_ftrace_ops" is successfully registered. Otherwise, another
> Kprobe will not try to register it again. Note that we could move the
> manipulation with this counter because it is accessed only under "kprobe_mutex".
> 
> Four, we should mark the probe as disabled if the ftrace stuff is not usable.
> It will be the correct status. Also it will prevent the unregistration code
> from producing another failure.
> 
> It looks more safe to disable the Kprobe directly in "kprobe_ftrace_ops". Note
> that we need to disable also all listed Kprobes in case of an aggregated probe.
> It would be enough to disable only the new one but we do not know which one it
> was. They should be in sync anyway.
> 

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thank you!

> Signed-off-by: Petr Mladek <pmladek@suse.cz>
> ---
>  kernel/kprobes.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index ee619929cf90..d1b9db690b9c 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -931,16 +931,33 @@ static int prepare_kprobe(struct kprobe *p)
>  /* Caller must lock kprobe_mutex */
>  static void arm_kprobe_ftrace(struct kprobe *p)
>  {
> +	struct kprobe *kp;
>  	int ret;
>  
>  	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
>  				   (unsigned long)p->addr, 0, 0);
> -	WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
> -	kprobe_ftrace_enabled++;
> -	if (kprobe_ftrace_enabled == 1) {
> +	if (WARN(ret < 0,
> +		 "Failed to arm kprobe-ftrace at %p (%d). The kprobe gets disabled.\n",
> +		 p->addr, ret))
> +		goto err_filter;
> +
> +	if (!kprobe_ftrace_enabled) {
>  		ret = register_ftrace_function(&kprobe_ftrace_ops);
> -		WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
> +		if (WARN(ret < 0,
> +			 "Failed to init kprobe-ftrace (%d). The probe at %p gets disabled\n",
> +			 ret, p->addr))
> +			goto err_function;
>  	}
> +	kprobe_ftrace_enabled++;
> +	return;
> +
> +err_function:
> +	ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);
> +err_filter:
> +	p->flags |= KPROBE_FLAG_DISABLED;
> +	if (kprobe_aggrprobe(p))
> +		list_for_each_entry_rcu(kp, &p->list, list)
> +			kp->flags |= KPROBE_FLAG_DISABLED;
>  }
>  
>  /* Caller must lock kprobe_mutex */
> 


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