From patchwork Tue May 5 21:02:41 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: William Cohen X-Patchwork-Id: 6344191 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 95FD9BEEE1 for ; Tue, 5 May 2015 21:49:56 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 753042015A for ; Tue, 5 May 2015 21:49:55 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3D3C22014A for ; Tue, 5 May 2015 21:49:54 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1Ypkij-00078D-MI; Tue, 05 May 2015 21:49:41 +0000 Received: from mx1.redhat.com ([209.132.183.28]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1Ypkhy-0006eJ-DC for linux-arm-kernel@lists.infradead.org; Tue, 05 May 2015 21:48:59 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t45L2hdb018821 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 5 May 2015 17:02:44 -0400 Received: from [10.13.129.68] (dhcp129-68.rdu.redhat.com [10.13.129.68]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t45L2fSD018029; Tue, 5 May 2015 17:02:41 -0400 Message-ID: <55492FF1.6040907@redhat.com> Date: Tue, 05 May 2015 17:02:41 -0400 From: William Cohen User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Will Deacon , David Long Subject: Re: [PATCH v6 0/6] arm64: Add kernel probes (kprobes) support References: <1429561187-3661-1-git-send-email-dave.long@linaro.org> <55363791.4070706@hitachi.com> <553AB222.50609@redhat.com> <553EF74D.8020706@redhat.com> <20150429102346.GK8236@arm.com> <55442BFA.50101@redhat.com> <554851CB.2010909@linaro.org> <20150505154830.GI1550@arm.com> In-Reply-To: <20150505154830.GI1550@arm.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150505_144854_548674_F62CE896 X-CRM114-Status: GOOD ( 34.13 ) X-Spam-Score: -5.0 (-----) Cc: "Jon Medhurst \(Tixy\)" , Russell King , Ananth N Mavinakayanahalli , Catalin Marinas , "linux-kernel@vger.kernel.org" , Anil S Keshavamurthy , "sandeepa.s.prabhu@gmail.com" , Masami Hiramatsu , Steve Capper , "davem@davemloft.net" , "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 05/05/2015 11:48 AM, Will Deacon wrote: > On Tue, May 05, 2015 at 06:14:51AM +0100, David Long wrote: >> On 05/01/15 21:44, William Cohen wrote: >>> Dave Long and I did some additional experimentation to better >>> understand what is condition causes the kernel to sometimes spew: >>> >>> Unexpected kernel single-step exception at EL1 >>> >>> The functioncallcount.stp test instruments the entry and return of >>> every function in the mm files, including kfree. In most cases the >>> arm64 trampoline_probe_handler just determines which return probe >>> instance matches the current conditions, runs the associated handler, >>> and recycles the return probe instance for another use by placing it >>> on a hlist. However, it is possible that a return probe instance has >>> been set up on function entry and the return probe is unregistered >>> before the return probe instance fires. In this case kfree is called >>> by the trampoline handler to remove the return probe instances related >>> to the unregistered kretprobe. This case where the the kprobed kfree >>> is called within the arm64 trampoline_probe_handler function trigger >>> the problem. >>> >>> The kprobe breakpoint for the kfree call from within the >>> trampoline_probe_handler is encountered and started, but things go >>> wrong when attempting the single step on the instruction. >>> >>> It took a while to trigger this problem with the sytemtap testsuite. >>> Dave Long came up with steps that reproduce this more quickly with a >>> probed function that is always called within the trampoline handler. >>> Trying the same on x86_64 doesn't trigger the problem. It appears >>> that the x86_64 code can handle a single step from within the >>> trampoline_handler. >>> >> >> I'm assuming there are no plans for supporting software breakpoint debug >> exceptions during processing of single-step exceptions, any time soon on >> arm64. Given that the only solution that I can come with for this is >> instead of making this orphaned kretprobe instance list exist only >> temporarily (in the scope of the kretprobe trampoline handler), make it >> always exist and kfree any items found on it as part of a periodic >> cleanup running outside of the handler context. I think these changes >> would still all be in archiecture-specific code. This doesn't feel to >> me like a bad solution. Does anyone think there is a simpler way out of >> this? > > Just to clarify, is the problem here the software breakpoint exception, > or trying to step the faulting instruction whilst we were already handling > a step? > > I think I'd be inclined to keep the code run in debug context to a minimum. > We already can't block there, and the more code we add the more black spots > we end up with in the kernel itself. The alternative would be to make your > kprobes code re-entrant, but that sounds like a nightmare. > > You say this works on x86. How do they handle it? Is the nested probe > on kfree ignored or handled? > > Will > Hi Dave and Will, The attached patch attempts to eliminate the need for the breakpoint in the trampoline. It is modeled after the x86_64 code and just saves the register state, calls the trampoline handler, and then fixes the return address. The code compiles, but I have NOT verified that it works. It looks feasible to do things this way. In addition to avoiding the possible issue with a kretprobe on kfree it would also make the kretprobes faster because it would avoid the breakpoint exception and the associated kprobe handling in the trampoline. -Will diff --git a/arch/arm64/kernel/kprobes-arm64.h b/arch/arm64/kernel/kprobes-arm64.h index ff8a55f..0b9987d 100644 --- a/arch/arm64/kernel/kprobes-arm64.h +++ b/arch/arm64/kernel/kprobes-arm64.h @@ -27,4 +27,40 @@ extern kprobes_pstate_check_t * const kprobe_condition_checks[16]; enum kprobe_insn __kprobes arm_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi); +#define SAVE_REGS_STRING\ + " stp x0, x1, [sp, #16 * 0]\n" \ + " stp x2, x3, [sp, #16 * 1]\n" \ + " stp x4, x5, [sp, #16 * 2]\n" \ + " stp x6, x7, [sp, #16 * 3]\n" \ + " stp x8, x9, [sp, #16 * 4]\n" \ + " stp x10, x11, [sp, #16 * 5]\n" \ + " stp x12, x13, [sp, #16 * 6]\n" \ + " stp x14, x15, [sp, #16 * 7]\n" \ + " stp x16, x17, [sp, #16 * 8]\n" \ + " stp x18, x19, [sp, #16 * 9]\n" \ + " stp x20, x21, [sp, #16 * 10]\n" \ + " stp x22, x23, [sp, #16 * 11]\n" \ + " stp x24, x25, [sp, #16 * 12]\n" \ + " stp x26, x27, [sp, #16 * 13]\n" \ + " stp x28, x29, [sp, #16 * 14]\n" \ + " str x30, [sp, #16 * 15]\n" + +#define RESTORE_REGS_STRING\ + " ldp x2, x3, [sp, #16 * 1]\n" \ + " ldp x4, x5, [sp, #16 * 2]\n" \ + " ldp x6, x7, [sp, #16 * 3]\n" \ + " ldp x8, x9, [sp, #16 * 4]\n" \ + " ldp x10, x11, [sp, #16 * 5]\n" \ + " ldp x12, x13, [sp, #16 * 6]\n" \ + " ldp x14, x15, [sp, #16 * 7]\n" \ + " ldp x16, x17, [sp, #16 * 8]\n" \ + " ldp x18, x19, [sp, #16 * 9]\n" \ + " ldp x20, x21, [sp, #16 * 10]\n" \ + " ldp x22, x23, [sp, #16 * 11]\n" \ + " ldp x24, x25, [sp, #16 * 12]\n" \ + " ldp x26, x27, [sp, #16 * 13]\n" \ + " ldp x28, x29, [sp, #16 * 14]\n" \ + " ldr x30, [sp, #16 * 15]\n" + + #endif /* _ARM_KERNEL_KPROBES_ARM64_H */ diff --git a/arch/arm64/kernel/kprobes.c b/arch/arm64/kernel/kprobes.c index 2b3ef17..f5dab2d 100644 --- a/arch/arm64/kernel/kprobes.c +++ b/arch/arm64/kernel/kprobes.c @@ -561,32 +561,27 @@ int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs) } /* - * Kretprobes: kernel return probes handling - * - * AArch64 mode does not support popping the PC value from the - * stack like on ARM 32-bit (ldmia {..,pc}), so atleast one - * register need to be used to achieve branching/return. - * It means return probes cannot return back to the original - * return address directly without modifying the register context. - * - * So like other architectures, we prepare a global routine - * with NOPs, which serve as trampoline address that hack away the - * function return, with the exact register context. - * Placing a kprobe on trampoline routine entry will trap again to - * execute return probe handlers and restore original return address - * in ELR_EL1, this way saved pt_regs still hold the original - * register values to be carried back to the caller. + * When a retprobed function returns, this code saves registers and + * calls trampoline_handler() runs, which calls the kretprobe's handler. */ -static void __used kretprobe_trampoline_holder(void) +static void __kprobes __used kretprobe_trampoline_holder(void) { - asm volatile (".global kretprobe_trampoline\n" - "kretprobe_trampoline:\n" - "NOP\n\t" - "NOP\n\t"); + asm volatile ( + ".global kretprobe_trampoline\n" + "kretprobe_trampoline: \n" + SAVE_REGS_STRING + "mov x0, sp\n" + "bl trampoline_handler\n" + /* Replace trampoline address in lr with actual + orig_ret_addr return address. */ + "str x0, [sp, #16 * 15]\n" + RESTORE_REGS_STRING + "ret\n" + : : : "memory"); } -static int __kprobes -trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs) + +static void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs) { struct kretprobe_instance *ri = NULL; struct hlist_head *head, empty_rp; @@ -647,7 +642,7 @@ trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs) } /* return 1 so that post handlers not called */ - return 1; + return (void *) orig_ret_addr; } void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, @@ -659,18 +654,7 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, regs->regs[30] = (long)&kretprobe_trampoline; } -static struct kprobe trampoline = { - .addr = (kprobe_opcode_t *) &kretprobe_trampoline, - .pre_handler = trampoline_probe_handler -}; - -int __kprobes arch_trampoline_kprobe(struct kprobe *p) -{ - return p->addr == (kprobe_opcode_t *) &kretprobe_trampoline; -} - int __init arch_init_kprobes(void) { - /* register trampoline for kret probe */ - return register_kprobe(&trampoline); + return 0; }