From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752942AbdBNKd2 (ORCPT ); Tue, 14 Feb 2017 05:33:28 -0500 Received: from smarthost01d.mail.zen.net.uk ([212.23.1.7]:37374 "EHLO smarthost01d.mail.zen.net.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752115AbdBNKc6 (ORCPT ); Tue, 14 Feb 2017 05:32:58 -0500 Message-ID: <1487068371.2978.6.camel@linaro.org> Subject: Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of multiple kretprobes From: "Jon Medhurst (Tixy)" To: Masami Hiramatsu , Russell King Cc: linux-kernel@vger.kernel.org, Peter Zijlstra , Ananth N Mavinakayanahalli , Ingo Molnar , Thomas Gleixner , "H . Peter Anvin" , Wang Nan , Catalin Marinas , Will Deacon , "David A . Long" , Sandeepa Prabhu Date: Tue, 14 Feb 2017 10:32:51 +0000 In-Reply-To: <148699834879.8505.2595570331866285215.stgit@devbox> References: <148699813647.8505.3671818093118978038.stgit@devbox> <148699834879.8505.2595570331866285215.stgit@devbox> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.4-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-Originating-smarthost01d-IP: [82.69.122.217] Feedback-ID: 82.69.122.217 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2017-02-14 at 00:05 +0900, Masami Hiramatsu wrote: > This is arm port of commit 737480a0d525 ("kprobes/x86: > Fix the return address of multiple kretprobes"). > > Fix the return address of subsequent kretprobes when multiple > kretprobes are set on the same function. > > For example: > > # cd /sys/kernel/debug/tracing > # echo "r:event1 sys_symlink" > kprobe_events > # echo "r:event2 sys_symlink" >> kprobe_events > # echo 1 > events/kprobes/enable > # ln -s /tmp/foo /tmp/bar > > (without this patch) > > # cat trace | grep -v ^# > ln-82 [000] dn.2 68.446525: event1: (kretprobe_trampoline+0x0/0x18 <- SyS_symlink) > ln-82 [000] dn.2 68.447831: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink) > > (with this patch) > > # cat trace | grep -v ^# > ln-81 [000] dn.1 39.463469: event1: (ret_fast_syscall+0x0/0x1c <- SyS_symlink) > ln-81 [000] dn.1 39.464701: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink) > > Signed-off-by: Masami Hiramatsu > Cc: KUMANO Syuhei > --- I don't fully understand this function, but I've checked that the ARM version now matches the x86 version (apart from the x86 specific register fixup and some comments). So, FWIW Acked-by: Jon Medhurst I ran the before and after test case in the commit log on ARM and verified the result is correct. I also tried running the ARM kprobe tests with these 3 fixes but the tests fail. However, they also fail without any of these changes, so I'll investigate that further... > arch/arm/probes/kprobes/core.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c > index 84989ae..023800a 100644 > --- a/arch/arm/probes/kprobes/core.c > +++ b/arch/arm/probes/kprobes/core.c > @@ -440,6 +440,7 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs) > struct hlist_node *tmp; > unsigned long flags, orig_ret_address = 0; > unsigned long trampoline_address = (unsigned long)&kretprobe_trampoline; > + kprobe_opcode_t *correct_ret_addr = NULL; > > INIT_HLIST_HEAD(&empty_rp); > kretprobe_hash_lock(current, &head, &flags); > @@ -462,14 +463,34 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs) > /* another task is sharing our hash bucket */ > continue; > > + orig_ret_address = (unsigned long)ri->ret_addr; > + > + if (orig_ret_address != trampoline_address) > + /* > + * This is the real return address. Any other > + * instances associated with this task are for > + * other calls deeper on the call stack > + */ > + break; > + } > + > + kretprobe_assert(ri, orig_ret_address, trampoline_address); > + > + correct_ret_addr = ri->ret_addr; > + hlist_for_each_entry_safe(ri, tmp, head, hlist) { > + if (ri->task != current) > + /* another task is sharing our hash bucket */ > + continue; > + > + orig_ret_address = (unsigned long)ri->ret_addr; > if (ri->rp && ri->rp->handler) { > __this_cpu_write(current_kprobe, &ri->rp->kp); > get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE; > + ri->ret_addr = correct_ret_addr; > ri->rp->handler(ri, regs); > __this_cpu_write(current_kprobe, NULL); > } > > - orig_ret_address = (unsigned long)ri->ret_addr; > recycle_rp_inst(ri, &empty_rp); > > if (orig_ret_address != trampoline_address) > @@ -481,7 +502,6 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs) > break; > } > > - kretprobe_assert(ri, orig_ret_address, trampoline_address); > kretprobe_hash_unlock(current, &flags); > > hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) { >