Re: [PATCH ftrace/core v6 4/5] kprobes: Set IPMODIFY flag only if the probe can change regs->ip

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 21 Nov 2014 05:25:30 -0500
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> Set FTRACE_OPS_FL_IPMODIFY flag only for the probes which can change
> regs->ip, which has kprobe->break_handler.
> Currently we can not put jprobe and another ftrace handler which
> changes regs->ip on the same function because all kprobes have
> FTRACE_OPS_FL_IPMODIFY flag. This removes FTRACE_OPS_FL_IPMODIFY
> flag from kprobes and only when the user uses jprobe (or the
> kprobe.break_handler != NULL) we add additinal ftrace_ops with
> FTRACE_OPS_FL_IPMODIFY on target function.
> 
> Note about the implementation: This uses a dummy ftrace_ops to
> reserve IPMODIFY flag on the given ftrace address, for the case
> that we have a enabled kprobe on a function entry and a jprobe
> is added on the same point. In that case, we already have a
> ftrace_ops without IPMODIFY flag on the entry, and we have to
> add another ftrace_ops with IPMODIFY on the same address.
> If we put a same handler on both ftrace_ops, the handler can
> be called twice on that entry until the first one is removed.
> This means that the kprobe and the jprobe are called twice too,
> and that will not what kprobes expected.
> Thus I added a dummy ftrace_ops just for reserving IPMODIFY flag.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> ---
> Changes in v4:
>  - Increment refcounter after succeeded to register ftrace_ops.
> 
> Changes in v3:
>  - Update __ftrace_add/remove_filter_ip() according to
>    Namhyng's comments (thanks!)
>  - Split out regs->ip recovering code from this patch.
> ---
>  Documentation/kprobes.txt |   12 ++--
>  kernel/kprobes.c          |  125 +++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 114 insertions(+), 23 deletions(-)
> 
> diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
> index 4227ec2..eb03efc 100644
> --- a/Documentation/kprobes.txt
> +++ b/Documentation/kprobes.txt
> @@ -264,15 +264,13 @@ stop-machine method that ksplice uses for supporting a CONFIG_PREEMPT=y
>  kernel.
>  
>  NOTE for geeks:
> -The jump optimization changes the kprobe's pre_handler behavior.
> -Without optimization, the pre_handler can change the kernel's execution
> +The jump optimization (and ftrace-based kprobes) changes the kprobe's
> +pre_handler behavior.
> +Without optimizations, the pre_handler can change the kernel's execution
>  path by changing regs->ip and returning 1.  However, when the probe
>  is optimized, that modification is ignored.  Thus, if you want to
> -tweak the kernel's execution path, you need to suppress optimization,
> -using one of the following techniques:
> -- Specify an empty function for the kprobe's post_handler or break_handler.
> - or
> -- Execute 'sysctl -w debug.kprobes_optimization=n'
> +tweak the kernel's execution path, you need to suppress optimization or
> +notify your handler will modify regs->ip by setting p->break_handler.
>  
>  1.5 Blacklist
>  
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 831978c..4b4b7c5 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -915,10 +915,93 @@ static struct kprobe *alloc_aggr_kprobe(struct kprobe *p)
>  #ifdef CONFIG_KPROBES_ON_FTRACE
>  static struct ftrace_ops kprobe_ftrace_ops __read_mostly = {
>  	.func = kprobe_ftrace_handler,
> -	.flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY,
> +	.flags = FTRACE_OPS_FL_SAVE_REGS,
>  };
>  static int kprobe_ftrace_enabled;
>  
> +static void kprobe_ftrace_stub(unsigned long a0, unsigned long a1,
> +			struct ftrace_ops *op, struct pt_regs *regs)
> +{
> +	/* Do nothing. This is just a dummy handler */
> +}

Feel free to just use ftrace_stub instead. That's what it's there for.


> +
> +/* This is only for checking conflict with other ftrace users */
> +static struct ftrace_ops kprobe_ipmod_ftrace_ops __read_mostly = {
> +	.func = kprobe_ftrace_stub,
> +	.flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY,
> +};
> +static int kprobe_ipmod_ftrace_enabled;
> +
> +static int __ftrace_add_filter_ip(struct ftrace_ops *ops, unsigned long ip,
> +				  int *ref)
> +{
> +	int ret;
> +
> +	/* Try to set given ip to filter */
> +	ret = ftrace_set_filter_ip(ops, ip, 0, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (*ref == 0) {
> +		ret = register_ftrace_function(ops);
> +		if (ret < 0) {
> +			/* Rollback the filter */
> +			ftrace_set_filter_ip(ops, ip, 1, 0);
> +			goto out;

Why the goto out, and not just return ret?

> +		}
> +	}
> +	(*ref)++;
> +
> +out:
> +	return ret;

Probably could just return 0 here.

Rest looks fine.

-- Steve

> +}
> +
> +static int __ftrace_remove_filter_ip(struct ftrace_ops *ops, unsigned long ip,
> +				     int *ref)
> +{
> +	int ret;
> +
> +	if (*ref == 1) {
> +		ret = unregister_ftrace_function(ops);
> +		if (ret < 0)
> +			return ret;
> +		/*Ignore failure, because it is already unregistered */
> +		ftrace_set_filter_ip(ops, ip, 1, 0);
> +	} else {
> +		/* Try to remove given ip to filter */
> +		ret = ftrace_set_filter_ip(ops, ip, 1, 0);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	(*ref)--;
> +
> +	return 0;
> +}
> +