Re: [PATCH v2] kprobes: Disable Kprobe when ftrace arming fails

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 16 Mar 2015 16:27:47 +0100
Petr Mladek <pmladek@suse.cz> 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.
> 
> 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.
> 

I can pull this into my tree. But there's one little nit below.

> Signed-off-by: Petr Mladek <pmladek@suse.cz>
> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
> Hi,
> 
> I resend this patch separately and have just added the acked by Masami.
> 
> Best Regards,
> Petr
> 
>  kernel/kprobes.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index c90e417bb963..54b22db084f3 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -932,16 +932,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;

The if statement should have brackets. No bracket if and for statements
should only be used when there's a singe simple non complex line below.
This is not the case.

Thanks,

-- Steve


>  }
>  
>  /* Caller must lock kprobe_mutex */