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 F3A2FC4332F for ; Fri, 2 Dec 2022 00:44:18 +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:Mime-Version:References:In-Reply-To: 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=GnPUw7Lg5t9qpLiK3WIgNtP8ff2Cgv1jobeK4FjaRhQ=; b=40K3MAIhOaWG9d FXBUEyrlnQBm6CV0X1qW6uBtXqluerjNnsdNaH7LOk2CV5FIJi4hMR33+k6pdIwgQOUTwrK2sAJqa tBp4i9Lts+p8bDateOb1HCT/fIfPsr8rq0y9hovacgRSBnK972C/RrS6o8K3d3ZW47uMA3qJMmQl4 TiyKJ4z6z8pDcq5FV0KoBKTD0HXjlfSI9vBT6qfgDOL9B46Cb+U6LQrFoP8hoA83gEH43+vU8mVbN R9+m2v3AgNLJcwbsw+UIcbznmpckARP5aKiBwoXnBc3e7L7kLrJK+4Bif6usMYIWuH+bEgziOSBbN k8n7+9wKVSEuM3HwRatQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1p0u90-00C2Bz-C0; Fri, 02 Dec 2022 00:43:10 +0000 Received: from ams.source.kernel.org ([145.40.68.75]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1p0u8w-00C2At-MQ for linux-arm-kernel@lists.infradead.org; Fri, 02 Dec 2022 00:43:08 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 00A1CB8206A; Fri, 2 Dec 2022 00:43:05 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id EC501C433D6; Fri, 2 Dec 2022 00:43:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1669941782; bh=vrpHgsXnWq/mFSpt5S6Islz7chU8nc7CTyA5NLJO2CQ=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Yi1Vp5+azdR9L5gJ9EOmJbTLCfvxt0poEKYHftsX3TPvWgVHg6kFOtwji8/F8l3se xFRYA0OILK8snjGAQ8jKvwyMQAzRaiRxDdZcNUP4P3eERu6prJXRAIfQJhPfP0/A+7 Bcb+Fe7kWjYK01cJbvTz/YsAhlenewYuphyyl7nNVaIr+N524BUO0qAX64zpsZehIz YLJZ1kQVCySDwuKREvkpHRGEtWRYt7B72zS3SeK4zV3hX4obFUT9qHer8Sr0dhyNgX 4zAoeqNkEb7rPgPc83CMgEAe0oQ2SFkp7u0TsdiurKc7vBT+3eawtiEgpm8rGBhINr ex37831vU5mwA== Date: Fri, 2 Dec 2022 09:42:58 +0900 From: Masami Hiramatsu (Google) To: Mark Rutland 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: <20221202094258.1ca06a6f49dcc3878e18c46a@kernel.org> In-Reply-To: References: <166990553243.253128.13594802750635478633.stgit@devnote3> <166990556124.253128.2968612748605960211.stgit@devnote3> <20221202010713.15d9c3bf5520b17a3df8e290@kernel.org> X-Mailer: Sylpheed 3.8.0beta1 (GTK+ 2.24.33; 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-20221201_164307_060749_0F3F1036 X-CRM114-Status: GOOD ( 51.15 ) 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, 1 Dec 2022 17:21:55 +0000 Mark Rutland wrote: > On Fri, Dec 02, 2022 at 01:07:13AM +0900, Masami Hiramatsu wrote: > > On Thu, 1 Dec 2022 15:08:52 +0000 > > Mark Rutland wrote: > > > > > 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. > > > > Interesting idea... and it also need many changes in kprobe itself. > > > > > > > > In case we do try to change that in future, it would be good to have a comment > > > somewhere to that effect. > > > > Hmm, it would fundamentally change the assumptions that kprobes relies on, > > and would require a lot of thought again. (e.g. current running kprobe is > > stored in per-cpu variable, it should be per-task. etc.) > > Ah; I had not considered that. > > Feel free to ignore the above; with the comments as below: > > Acked-by: Mark Rutland OK, Thanks! > > Thanks, > Mark. > > > > > > > > > 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. > > > */ > > > > OK. > > > > > > > > > + 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. > > > > OK, I'll update both in v2. > > > > Thank you! > > > > > > > > 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). > > > > > > > > > > -- > > Masami Hiramatsu (Google) -- Masami Hiramatsu (Google) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel