From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754615AbcGTTIt (ORCPT ); Wed, 20 Jul 2016 15:08:49 -0400 Received: from mail-qk0-f174.google.com ([209.85.220.174]:33625 "EHLO mail-qk0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751427AbcGTTIq (ORCPT ); Wed, 20 Jul 2016 15:08:46 -0400 Subject: Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support To: Marc Zyngier , 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> <578F4608.4030901@arm.com> 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: David Long Message-ID: <578FCC3A.6090805@linaro.org> Date: Wed, 20 Jul 2016 15:08:42 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <578F4608.4030901@arm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/20/2016 05:36 AM, Marc Zyngier wrote: > 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. Yes, this describes how it originally worked but I changed it based on earlier feedback. Apparently this comment did not get updated. > - 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"); > } > OK > though hijacking SP in the middle of a C function still feels pretty fragile. > > Thanks, > > M. > Thanks, -dl From mboxrd@z Thu Jan 1 00:00:00 1970 From: dave.long@linaro.org (David Long) Date: Wed, 20 Jul 2016 15:08:42 -0400 Subject: [PATCH v15 04/10] arm64: Kprobes with single stepping support In-Reply-To: <578F4608.4030901@arm.com> References: <1467995754-32508-1-git-send-email-dave.long@linaro.org> <1467995754-32508-5-git-send-email-dave.long@linaro.org> <578F4608.4030901@arm.com> Message-ID: <578FCC3A.6090805@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 07/20/2016 05:36 AM, Marc Zyngier wrote: > 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. Yes, this describes how it originally worked but I changed it based on earlier feedback. Apparently this comment did not get updated. > - 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"); > } > OK > though hijacking SP in the middle of a C function still feels pretty fragile. > > Thanks, > > M. > Thanks, -dl