From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6F08DC2D0A3 for ; Thu, 29 Oct 2020 23:36:18 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B1CDC20739 for ; Thu, 29 Oct 2020 23:36:17 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="3hbDBQbH"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="VGVrGcL6" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B1CDC20739 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Mime-Version:References:In-Reply-To:Message-Id: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=CPDZ1Dc6TQnLcL3bIUoAcvRndHm1zMrPt1DdPUuGEQE=; b=3hbDBQbHgMssMgcCkCoUb+YPc 64eQVqE/By0EmXXPktieUCdvun3rEKO292GAqi24u6WLwB+lTL8+nIc6KbREKoG4L+QnKkLFRb63i OQmzp54ir8CKxizEWaGHLw0qH4slias/NLiWD4Pw60R3cC+Mjbi8E5qtUQZcOjI0EaIbNEcIS2sw5 lH551Izupx1XuGgmpeyqvip85Ug9iwGQQjz5mmvalqyqVPgMD/cqmoGwMTK2hXBIh1N4Xwq3Vqr2K WEshrq/zwINV/q2cmK4O6gxUOU53l5yzCx601BtwdLkOagi0TbwfxjhYta82Aeh0fz0JMwQRWedkl 1XCUvlpMw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kYHRO-0007Jx-Bu; Thu, 29 Oct 2020 23:34:46 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kYHRK-0007JK-C8 for linux-arm-kernel@lists.infradead.org; Thu, 29 Oct 2020 23:34:43 +0000 Received: from devnote2 (NE2965lan1.rev.em-net.ne.jp [210.141.244.193]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id C419020739; Thu, 29 Oct 2020 23:34:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1604014480; bh=4zcbGVRj3Nk05mLNiGPMupagAMhqLwBJ+j8/BGgVTm8=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=VGVrGcL6wTgMmcgEAa7ibJHmxqZziQKj4mKcCbsJCQLZSDXsLb4I0dHniuPNtD9X9 XcZZI/SCVwD7aWOwQBEzxQnroenvtLyNG+t7+XV6rAaKADRHC6+8OAYIjs8WJ+cQlw j2PF6lODVF0EwsZZ81vInNVWkArKoDVVDxF5vcO0= Date: Fri, 30 Oct 2020 08:34:37 +0900 From: Masami Hiramatsu To: Jean-Philippe Brucker Subject: Re: [PATCH] arm64: kprobes: Use BRK instead of single-step when executing instructions out-of-line Message-Id: <20201030083437.89f8feea318f022437a02821@kernel.org> In-Reply-To: <20201029173440.117174-1-jean-philippe@linaro.org> References: <20201029173440.117174-1-jean-philippe@linaro.org> X-Mailer: Sylpheed 3.7.0 (GTK+ 2.24.32; x86_64-pc-linux-gnu) Mime-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201029_193442_629426_3B585826 X-CRM114-Status: GOOD ( 41.80 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: catalin.marinas@arm.com, will@kernel.org, mhiramat@kernel.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, 29 Oct 2020 18:34:42 +0100 Jean-Philippe Brucker wrote: > Commit 36dadef23fcc ("kprobes: Init kprobes in early_initcall") enabled > using kprobes from early_initcall. Unfortunately at this point the > hardware debug infrastructure is not operational. The OS lock may still > be locked, and the hardware watchpoints may have unknown values when > kprobe enables debug monitors to single-step instructions. > > Rather than using hardware single-step, append a BRK instruction after > the instruction to be executed out-of-line. Ok, this looks good to me. One comment is that we can remove ss_ctx too. ss_ctx has ss_pending bit and match_addr, those are redundant because those can be replaced by KPROBE_HIT_SS and &cur_kprobe->ainsn.api.insn[1] respectively. But that should be done in another patch. To fix the bug, I think this is good. Acked-by: Masami Hiramatsu > Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall") > Suggested-by: Will Deacon > Signed-off-by: Jean-Philippe Brucker > --- > > An alternative to fix [1]. I haven't done uprobes yet since they don't > suffer the same problem, but could add it to the bottom of my list. > Lightly tested with kprobe smoke test and the BPF selftests. > Interestingly on Seattle when running the "overhead" BPF selftest, that > triggers a kprobe a bunch of times, I see a 20-30% improvement with this > patch. I'm guessing it's the cost of touching the debug sysregs? Interesting :) I think that is the cost of the debug exception itself. I guess there might be a cost to route the debug logic. Thank you, > [1] https://lore.kernel.org/linux-arm-kernel/20201026172907.1468294-1-jean-philippe@linaro.org/ > --- > arch/arm64/include/asm/brk-imm.h | 2 + > arch/arm64/include/asm/debug-monitors.h | 1 + > arch/arm64/include/asm/kprobes.h | 2 +- > arch/arm64/kernel/probes/kprobes.c | 56 +++++++++---------------- > 4 files changed, 24 insertions(+), 37 deletions(-) > > diff --git a/arch/arm64/include/asm/brk-imm.h b/arch/arm64/include/asm/brk-imm.h > index e3d47b52161d..ec7720dbe2c8 100644 > --- a/arch/arm64/include/asm/brk-imm.h > +++ b/arch/arm64/include/asm/brk-imm.h > @@ -10,6 +10,7 @@ > * #imm16 values used for BRK instruction generation > * 0x004: for installing kprobes > * 0x005: for installing uprobes > + * 0x006: for kprobe software single-step > * Allowed values for kgdb are 0x400 - 0x7ff > * 0x100: for triggering a fault on purpose (reserved) > * 0x400: for dynamic BRK instruction > @@ -19,6 +20,7 @@ > */ > #define KPROBES_BRK_IMM 0x004 > #define UPROBES_BRK_IMM 0x005 > +#define KPROBES_BRK_SS_IMM 0x006 > #define FAULT_BRK_IMM 0x100 > #define KGDB_DYN_DBG_BRK_IMM 0x400 > #define KGDB_COMPILED_DBG_BRK_IMM 0x401 > diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h > index 0b298f48f5bf..657c921fd784 100644 > --- a/arch/arm64/include/asm/debug-monitors.h > +++ b/arch/arm64/include/asm/debug-monitors.h > @@ -53,6 +53,7 @@ > > /* kprobes BRK opcodes with ESR encoding */ > #define BRK64_OPCODE_KPROBES (AARCH64_BREAK_MON | (KPROBES_BRK_IMM << 5)) > +#define BRK64_OPCODE_KPROBES_SS (AARCH64_BREAK_MON | (KPROBES_BRK_SS_IMM << 5)) > /* uprobes BRK opcodes with ESR encoding */ > #define BRK64_OPCODE_UPROBES (AARCH64_BREAK_MON | (UPROBES_BRK_IMM << 5)) > > diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h > index 97e511d645a2..8699ce30f587 100644 > --- a/arch/arm64/include/asm/kprobes.h > +++ b/arch/arm64/include/asm/kprobes.h > @@ -16,7 +16,7 @@ > #include > > #define __ARCH_WANT_KPROBES_INSN_SLOT > -#define MAX_INSN_SIZE 1 > +#define MAX_INSN_SIZE 2 > > #define flush_insn_slot(p) do { } while (0) > #define kretprobe_blacklist_size 0 > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c > index deba738142ed..ec1446ceacc9 100644 > --- a/arch/arm64/kernel/probes/kprobes.c > +++ b/arch/arm64/kernel/probes/kprobes.c > @@ -36,21 +36,25 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); > static void __kprobes > post_kprobe_handler(struct kprobe_ctlblk *, struct pt_regs *); > > -static int __kprobes patch_text(kprobe_opcode_t *addr, u32 opcode) > +static int __kprobes patch_text(kprobe_opcode_t *addr, u32 opc1, u32 opc2) > { > - void *addrs[1]; > - u32 insns[1]; > + void *addrs[2]; > + u32 insns[2]; > > addrs[0] = addr; > - insns[0] = opcode; > + insns[0] = opc1; > + if (opc2) { > + addrs[1] = addr + 1; > + insns[1] = opc2; > + } > > - return aarch64_insn_patch_text(addrs, insns, 1); > + return aarch64_insn_patch_text(addrs, insns, opc2 ? 2 : 1); > } > > static void __kprobes arch_prepare_ss_slot(struct kprobe *p) > { > /* prepare insn slot */ > - patch_text(p->ainsn.api.insn, p->opcode); > + patch_text(p->ainsn.api.insn, p->opcode, BRK64_OPCODE_KPROBES_SS); > > flush_icache_range((uintptr_t) (p->ainsn.api.insn), > (uintptr_t) (p->ainsn.api.insn) + > @@ -128,13 +132,13 @@ void *alloc_insn_page(void) > /* arm kprobe: install breakpoint in text */ > void __kprobes arch_arm_kprobe(struct kprobe *p) > { > - patch_text(p->addr, BRK64_OPCODE_KPROBES); > + patch_text(p->addr, BRK64_OPCODE_KPROBES, 0); > } > > /* disarm kprobe: remove breakpoint from text */ > void __kprobes arch_disarm_kprobe(struct kprobe *p) > { > - patch_text(p->addr, p->opcode); > + patch_text(p->addr, p->opcode, 0); > } > > void __kprobes arch_remove_kprobe(struct kprobe *p) > @@ -163,20 +167,14 @@ static void __kprobes set_current_kprobe(struct kprobe *p) > } > > /* > - * Interrupts need to be disabled before single-step mode is set, and not > - * reenabled until after single-step mode ends. > - * Without disabling interrupt on local CPU, there is a chance of > - * interrupt occurrence in the period of exception return and start of > - * out-of-line single-step, that result in wrongly single stepping > - * into the interrupt handler. > + * Keep interrupts disabled while executing the instruction out-of-line. Since > + * the kprobe state is per-CPU, we can't afford to be migrated. > */ > static void __kprobes kprobes_save_local_irqflag(struct kprobe_ctlblk *kcb, > struct pt_regs *regs) > { > kcb->saved_irqflag = regs->pstate & DAIF_MASK; > regs->pstate |= PSR_I_BIT; > - /* Unmask PSTATE.D for enabling software step exceptions. */ > - regs->pstate &= ~PSR_D_BIT; > } > > static void __kprobes kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb, > @@ -219,10 +217,7 @@ static void __kprobes setup_singlestep(struct kprobe *p, > slot = (unsigned long)p->ainsn.api.insn; > > set_ss_context(kcb, slot); /* mark pending ss */ > - > - /* IRQs and single stepping do not mix well. */ > kprobes_save_local_irqflag(kcb, regs); > - kernel_enable_single_step(regs); > instruction_pointer_set(regs, slot); > } else { > /* insn simulation */ > @@ -273,12 +268,8 @@ post_kprobe_handler(struct kprobe_ctlblk *kcb, struct pt_regs *regs) > } > /* call post handler */ > kcb->kprobe_status = KPROBE_HIT_SSDONE; > - if (cur->post_handler) { > - /* post_handler can hit breakpoint and single step > - * again, so we enable D-flag for recursive exception. > - */ > + if (cur->post_handler) > cur->post_handler(cur, regs, 0); > - } > > reset_current_kprobe(); > } > @@ -302,8 +293,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr) > if (!instruction_pointer(regs)) > BUG(); > > - kernel_disable_single_step(); > - > if (kcb->kprobe_status == KPROBE_REENTER) > restore_previous_kprobe(kcb); > else > @@ -365,10 +354,6 @@ static void __kprobes kprobe_handler(struct pt_regs *regs) > * pre-handler and it returned non-zero, it will > * modify the execution path and no need to single > * stepping. Let's just reset current kprobe and exit. > - * > - * pre_handler can hit a breakpoint and can step thru > - * before return, keep PSTATE D-flag enabled until > - * pre_handler return back. > */ > if (!p->pre_handler || !p->pre_handler(p, regs)) { > setup_singlestep(p, regs, kcb, 0); > @@ -399,7 +384,7 @@ kprobe_ss_hit(struct kprobe_ctlblk *kcb, unsigned long addr) > } > > static int __kprobes > -kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr) > +kprobe_breakpoint_ss_handler(struct pt_regs *regs, unsigned int esr) > { > struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); > int retval; > @@ -409,16 +394,15 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr) > > if (retval == DBG_HOOK_HANDLED) { > kprobes_restore_local_irqflag(kcb, regs); > - kernel_disable_single_step(); > - > post_kprobe_handler(kcb, regs); > } > > return retval; > } > > -static struct step_hook kprobes_step_hook = { > - .fn = kprobe_single_step_handler, > +static struct break_hook kprobes_break_ss_hook = { > + .imm = KPROBES_BRK_SS_IMM, > + .fn = kprobe_breakpoint_ss_handler, > }; > > static int __kprobes > @@ -486,7 +470,7 @@ int __kprobes arch_trampoline_kprobe(struct kprobe *p) > int __init arch_init_kprobes(void) > { > register_kernel_break_hook(&kprobes_break_hook); > - register_kernel_step_hook(&kprobes_step_hook); > + register_kernel_break_hook(&kprobes_break_ss_hook); > > return 0; > } > -- > 2.29.1 > -- Masami Hiramatsu _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel