Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752729AbaKUUQI (ORCPT ); Fri, 21 Nov 2014 15:16:08 -0500 Received: from cdptpa-outbound-snat.email.rr.com ([107.14.166.226]:22446 "EHLO cdptpa-oedge-vip.email.rr.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750863AbaKUUQE (ORCPT ); Fri, 21 Nov 2014 15:16:04 -0500 Date: Fri, 21 Nov 2014 15:15:48 -0500 From: Steven Rostedt To: Masami Hiramatsu Cc: Josh Poimboeuf , Ananth N Mavinakayanahalli , Jiri Kosina , Seth Jennings , Linux Kernel Mailing List , Petr Mladek , Vojtech Pavlik , Namhyung Kim , Miroslav Benes , Ingo Molnar Subject: Re: [PATCH ftrace/core v6 4/5] kprobes: Set IPMODIFY flag only if the probe can change regs->ip Message-ID: <20141121151548.7bd8bcd9@gandalf.local.home> In-Reply-To: <20141121102530.11844.41626.stgit@localhost.localdomain> References: <20141121102502.11844.82696.stgit@localhost.localdomain> <20141121102530.11844.41626.stgit@localhost.localdomain> X-Mailer: Claws Mail 3.10.1 (GTK+ 2.24.25; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-RR-Connecting-IP: 107.14.168.118:25 X-Cloudmark-Score: 0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 21 Nov 2014 05:25:30 -0500 Masami Hiramatsu 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 > Cc: Ananth N Mavinakayanahalli > Cc: Steven Rostedt > Cc: Josh Poimboeuf > Cc: Namhyung Kim > --- > 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; > +} > + -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/