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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 719DCC43217 for ; Thu, 1 Dec 2022 15:10:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc: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=Fnu1t1EI/0vfc7CXsKWYwmU67sy2kyTgzbrDQOHTYXg=; b=I7JbotDuc97dP0 64ziwbh4pBw0ygMp0jy5HXa1gsyxn97LWaHe6uG148vhmdDSanHWJZv4RyNCK49y29KmJEOzuJKjY WRZPEyJNK8bD44rdul4s79AFg+DRhD7PhNYxR9VPpCZFfi/MtY9+FZB0hiAON9HPsv2WG0vj5BdRZ X/jwlliy/uMctPXRRua/noqO2qTsI/9/zi+0UZy3wqsTxtKvRf5V5YpNCTP7dNOAQZTk/0uEcuFy5 CBf7iRqjQJ7HiA7taowsMz93trexj8hP0cYfVJfeoLBt1ZgrnaGpHnxlfag/7yU4XMP7zBQ10tj6H N/j9jIdm23Cb1jz8Nr6w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1p0lBO-0087c0-Pp; Thu, 01 Dec 2022 15:09:02 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1p0lBK-0087aj-LL for linux-arm-kernel@lists.infradead.org; Thu, 01 Dec 2022 15:09:00 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3D8681474; Thu, 1 Dec 2022 07:09:04 -0800 (PST) Received: from FVFF77S0Q05N (unknown [10.57.38.177]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C58CF3F67D; Thu, 1 Dec 2022 07:08:55 -0800 (PST) Date: Thu, 1 Dec 2022 15:08:52 +0000 From: Mark Rutland To: "Masami Hiramatsu (Google)" Cc: Catalin Marinas , Will Deacon , Mark Brown , Kalesh Singh , Marc Zyngier , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Sandeepa Prabhu Subject: Re: [PATCH 3/3] arm64: kprobes: Return DBG_HOOK_ERROR if kprobes can not handle a BRK Message-ID: References: <166990553243.253128.13594802750635478633.stgit@devnote3> <166990556124.253128.2968612748605960211.stgit@devnote3> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <166990556124.253128.2968612748605960211.stgit@devnote3> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221201_070858_827071_4C9780ED X-CRM114-Status: GOOD ( 34.35 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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, Dec 01, 2022 at 11:39:21PM +0900, Masami Hiramatsu (Google) wrote: > From: Masami Hiramatsu (Google) > > Return DBG_HOOK_ERROR if kprobes can not handle a BRK because it > fails to find a kprobe corresponding to the address. > > Since arm64 kprobes uses stop_machine based text patching for removing > BRK, it ensures all running kprobe_break_handler() is done at that point. > And after removing the BRK, it removes the kprobe from its hash list. > Thus, if the kprobe_break_handler() fails to find kprobe from hash list, > there is a bug. IIUC this relies on BRK handling not being preemptible, which is something we've repeatedly considered changing along with a bunch of other debug exception handling. In case we do try to change that in future, it would be good to have a comment somewhere to that effect. I think there are other ways we could synchronise against that (e.g. using RCU tasks rude) if we ever do that, and this patch looks good to me. > > Signed-off-by: Masami Hiramatsu (Google) > --- > arch/arm64/kernel/probes/kprobes.c | 79 +++++++++++++++++------------------- > 1 file changed, 37 insertions(+), 42 deletions(-) > > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c > index d2ae37f89774..ea56b22d4da8 100644 > --- a/arch/arm64/kernel/probes/kprobes.c > +++ b/arch/arm64/kernel/probes/kprobes.c > @@ -298,7 +298,8 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr) > return 0; > } > > -static void __kprobes kprobe_handler(struct pt_regs *regs) > +static int __kprobes > +kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr) > { > struct kprobe *p, *cur_kprobe; > struct kprobe_ctlblk *kcb; > @@ -308,39 +309,45 @@ static void __kprobes kprobe_handler(struct pt_regs *regs) > cur_kprobe = kprobe_running(); > > p = get_kprobe((kprobe_opcode_t *) addr); > + if (WARN_ON_ONCE(!p)) { > + /* > + * Something went wrong. This must be put by kprobe, but we > + * could not find corresponding kprobes. Let the kernel handle > + * this error case. > + */ Could we make this: /* * Something went wrong. This BRK used an immediate reserved * for kprobes, but we couldn't find any corresponding probe. */ > + return DBG_HOOK_ERROR; > + } > > - if (p) { > - if (cur_kprobe) { > - if (reenter_kprobe(p, regs, kcb)) > - return; > - } else { > - /* Probe hit */ > - set_current_kprobe(p); > - kcb->kprobe_status = KPROBE_HIT_ACTIVE; > - > - /* > - * If we have no pre-handler or it returned 0, we > - * continue with normal processing. If we have a > - * 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. > - */ > - if (!p->pre_handler || !p->pre_handler(p, regs)) { > - setup_singlestep(p, regs, kcb, 0); > - } else > - reset_current_kprobe(); > - } > + if (cur_kprobe) { > + /* Hit a kprobe inside another kprobe */ > + if (!reenter_kprobe(p, regs, kcb)) > + return DBG_HOOK_ERROR; > + } else { > + /* Probe hit */ > + set_current_kprobe(p); > + kcb->kprobe_status = KPROBE_HIT_ACTIVE; > + > + /* > + * If we have no pre-handler or it returned 0, we > + * continue with normal processing. If we have a > + * 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. > + */ Minor wording nit: could we replace: no need to single stepping. With: not need to single-step. Thanks, Mark. > + if (!p->pre_handler || !p->pre_handler(p, regs)) > + setup_singlestep(p, regs, kcb, 0); > + else > + reset_current_kprobe(); > } > - /* > - * The breakpoint instruction was removed right > - * after we hit it. Another cpu has removed > - * either a probepoint or a debugger breakpoint > - * at this address. In either case, no further > - * handling of this interrupt is appropriate. > - * Return back to original instruction, and continue. > - */ > + > + return DBG_HOOK_HANDLED; > } > > +static struct break_hook kprobes_break_hook = { > + .imm = KPROBES_BRK_IMM, > + .fn = kprobe_breakpoint_handler, > +}; > + > static int __kprobes > kprobe_breakpoint_ss_handler(struct pt_regs *regs, unsigned long esr) > { > @@ -365,18 +372,6 @@ static struct break_hook kprobes_break_ss_hook = { > .fn = kprobe_breakpoint_ss_handler, > }; > > -static int __kprobes > -kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr) > -{ > - kprobe_handler(regs); > - return DBG_HOOK_HANDLED; > -} > - > -static struct break_hook kprobes_break_hook = { > - .imm = KPROBES_BRK_IMM, > - .fn = kprobe_breakpoint_handler, > -}; > - > /* > * Provide a blacklist of symbols identifying ranges which cannot be kprobed. > * This blacklist is exposed to userspace via debugfs (kprobes/blacklist). > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel