From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753240AbcGTJgV (ORCPT ); Wed, 20 Jul 2016 05:36:21 -0400 Received: from foss.arm.com ([217.140.101.70]:40606 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751466AbcGTJgS (ORCPT ); Wed, 20 Jul 2016 05:36:18 -0400 Subject: Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support To: David Long , Catalin Marinas , Huang Shijie , James Morse , Pratyush Anand , Sandeepa Prabhu , Will Deacon , William Cohen , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Steve Capper , Masami Hiramatsu , Li Bin References: <1467995754-32508-1-git-send-email-dave.long@linaro.org> <1467995754-32508-5-git-send-email-dave.long@linaro.org> Cc: Adam Buchbinder , =?UTF-8?Q?Alex_Benn=c3=a9e?= , Andrew Morton , Andrey Ryabinin , Ard Biesheuvel , Christoffer Dall , Daniel Thompson , Dave P Martin , Jens Wiklander , Jisheng Zhang , John Blackwood , Mark Rutland , Petr Mladek , Robin Murphy , Suzuki K Poulose , Vladimir Murzin , Yang Shi , Zi Shen Lim , yalin wang , Mark Brown From: Marc Zyngier X-Enigmail-Draft-Status: N1110 Organization: ARM Ltd Message-ID: <578F4608.4030901@arm.com> Date: Wed, 20 Jul 2016 10:36:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.7.0 MIME-Version: 1.0 In-Reply-To: <1467995754-32508-5-git-send-email-dave.long@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/07/16 17:35, David Long wrote: > From: Sandeepa Prabhu > > Add support for basic kernel probes(kprobes) and jump probes > (jprobes) for ARM64. > > Kprobes utilizes software breakpoint and single step debug > exceptions supported on ARM v8. > > A software breakpoint is placed at the probe address to trap the > kernel execution into the kprobe handler. > > ARM v8 supports enabling single stepping before the break exception > return (ERET), with next PC in exception return address (ELR_EL1). The > kprobe handler prepares an executable memory slot for out-of-line > execution with a copy of the original instruction being probed, and > enables single stepping. The PC is set to the out-of-line slot address > before the ERET. With this scheme, the instruction is executed with the > exact same register context except for the PC (and DAIF) registers. > > Debug mask (PSTATE.D) is enabled only when single stepping a recursive > kprobe, e.g.: during kprobes reenter so that probed instruction can be > single stepped within the kprobe handler -exception- context. > The recursion depth of kprobe is always 2, i.e. upon probe re-entry, > any further re-entry is prevented by not calling handlers and the case > counted as a missed kprobe). > > Single stepping from the x-o-l slot has a drawback for PC-relative accesses > like branching and symbolic literals access as the offset from the new PC > (slot address) may not be ensured to fit in the immediate value of > the opcode. Such instructions need simulation, so reject > probing them. > > Instructions generating exceptions or cpu mode change are rejected > for probing. > > Exclusive load/store instructions are rejected too. Additionally, the > code is checked to see if it is inside an exclusive load/store sequence > (code from Pratyush). > > System instructions are mostly enabled for stepping, except MSR/MRS > accesses to "DAIF" flags in PSTATE, which are not safe for > probing. > > This also changes arch/arm64/include/asm/ptrace.h to use > include/asm-generic/ptrace.h. > > Thanks to Steve Capper and Pratyush Anand for several suggested > Changes. > > Signed-off-by: Sandeepa Prabhu > Signed-off-by: David A. Long > Signed-off-by: Pratyush Anand > Acked-by: Masami Hiramatsu > --- [...] > +void __kprobes jprobe_return(void) > +{ > + struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); > + > + /* > + * Jprobe handler return by entering break exception, > + * encoded same as kprobe, but with following conditions > + * -a magic number in x0 to identify from rest of other kprobes. > + * -restore stack addr to original saved pt_regs > + */ > + asm volatile ("ldr x0, [%0]\n\t" > + "mov sp, x0\n\t" > + ".globl jprobe_return_break\n\t" > + "jprobe_return_break:\n\t" > + "brk %1\n\t" > + : > + : "r"(&kcb->jprobe_saved_regs.sp), > + "I"(BRK64_ESR_KPROBES) > + : "memory"); A couple of remarks here: - the comment seems wrong, as you load the stack pointer in X0, nothing else, and seem to identify the jprobe by looking at the PC, not X0. - using explicit registers is really ugly. How about something like this instead: diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index c89811d..823cf92 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -513,13 +513,12 @@ void __kprobes jprobe_return(void) * -a magic number in x0 to identify from rest of other kprobes. * -restore stack addr to original saved pt_regs */ - asm volatile ("ldr x0, [%0]\n\t" - "mov sp, x0\n\t" + asm volatile ("mov sp, %0\n\t" ".globl jprobe_return_break\n\t" "jprobe_return_break:\n\t" "brk %1\n\t" : - : "r"(&kcb->jprobe_saved_regs.sp), + : "r" (kcb->jprobe_saved_regs.sp), "I"(BRK64_ESR_KPROBES) : "memory"); } though hijacking SP in the middle of a C function still feels pretty fragile. Thanks, M. -- Jazz is not dead. It just smells funny... From mboxrd@z Thu Jan 1 00:00:00 1970 From: marc.zyngier@arm.com (Marc Zyngier) Date: Wed, 20 Jul 2016 10:36:08 +0100 Subject: [PATCH v15 04/10] arm64: Kprobes with single stepping support In-Reply-To: <1467995754-32508-5-git-send-email-dave.long@linaro.org> References: <1467995754-32508-1-git-send-email-dave.long@linaro.org> <1467995754-32508-5-git-send-email-dave.long@linaro.org> Message-ID: <578F4608.4030901@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 08/07/16 17:35, David Long wrote: > From: Sandeepa Prabhu > > Add support for basic kernel probes(kprobes) and jump probes > (jprobes) for ARM64. > > Kprobes utilizes software breakpoint and single step debug > exceptions supported on ARM v8. > > A software breakpoint is placed at the probe address to trap the > kernel execution into the kprobe handler. > > ARM v8 supports enabling single stepping before the break exception > return (ERET), with next PC in exception return address (ELR_EL1). The > kprobe handler prepares an executable memory slot for out-of-line > execution with a copy of the original instruction being probed, and > enables single stepping. The PC is set to the out-of-line slot address > before the ERET. With this scheme, the instruction is executed with the > exact same register context except for the PC (and DAIF) registers. > > Debug mask (PSTATE.D) is enabled only when single stepping a recursive > kprobe, e.g.: during kprobes reenter so that probed instruction can be > single stepped within the kprobe handler -exception- context. > The recursion depth of kprobe is always 2, i.e. upon probe re-entry, > any further re-entry is prevented by not calling handlers and the case > counted as a missed kprobe). > > Single stepping from the x-o-l slot has a drawback for PC-relative accesses > like branching and symbolic literals access as the offset from the new PC > (slot address) may not be ensured to fit in the immediate value of > the opcode. Such instructions need simulation, so reject > probing them. > > Instructions generating exceptions or cpu mode change are rejected > for probing. > > Exclusive load/store instructions are rejected too. Additionally, the > code is checked to see if it is inside an exclusive load/store sequence > (code from Pratyush). > > System instructions are mostly enabled for stepping, except MSR/MRS > accesses to "DAIF" flags in PSTATE, which are not safe for > probing. > > This also changes arch/arm64/include/asm/ptrace.h to use > include/asm-generic/ptrace.h. > > Thanks to Steve Capper and Pratyush Anand for several suggested > Changes. > > Signed-off-by: Sandeepa Prabhu > Signed-off-by: David A. Long > Signed-off-by: Pratyush Anand > Acked-by: Masami Hiramatsu > --- [...] > +void __kprobes jprobe_return(void) > +{ > + struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); > + > + /* > + * Jprobe handler return by entering break exception, > + * encoded same as kprobe, but with following conditions > + * -a magic number in x0 to identify from rest of other kprobes. > + * -restore stack addr to original saved pt_regs > + */ > + asm volatile ("ldr x0, [%0]\n\t" > + "mov sp, x0\n\t" > + ".globl jprobe_return_break\n\t" > + "jprobe_return_break:\n\t" > + "brk %1\n\t" > + : > + : "r"(&kcb->jprobe_saved_regs.sp), > + "I"(BRK64_ESR_KPROBES) > + : "memory"); A couple of remarks here: - the comment seems wrong, as you load the stack pointer in X0, nothing else, and seem to identify the jprobe by looking at the PC, not X0. - using explicit registers is really ugly. How about something like this instead: diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index c89811d..823cf92 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -513,13 +513,12 @@ void __kprobes jprobe_return(void) * -a magic number in x0 to identify from rest of other kprobes. * -restore stack addr to original saved pt_regs */ - asm volatile ("ldr x0, [%0]\n\t" - "mov sp, x0\n\t" + asm volatile ("mov sp, %0\n\t" ".globl jprobe_return_break\n\t" "jprobe_return_break:\n\t" "brk %1\n\t" : - : "r"(&kcb->jprobe_saved_regs.sp), + : "r" (kcb->jprobe_saved_regs.sp), "I"(BRK64_ESR_KPROBES) : "memory"); } though hijacking SP in the middle of a C function still feels pretty fragile. Thanks, M. -- Jazz is not dead. It just smells funny...