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 62182C43217 for ; Thu, 1 Dec 2022 16:08:27 +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=haV+nItXTLRTyHZz3gE/XLDjl2aUTPg+n/3h1ho/+I0=; b=ki0rgDiIsd3phP /6n1C9QJFfrGWjxqoiQXTwTFBt3ZX99Nl2v75Zh3VmZdPHcvNUEN9gF7yeio2r7BXaJWCjqqHJa0w l90ndPSa2XBXtMn1ZNi9MO+N0z0YUc3oK3kQ8M85LiOPmyL9A+u0eJJGeeK7ik7rGlTKOzq+zFlLG KF8Fe3II0yozzXiuKy41RMfG5EiVt5jKZnYkt8FuCeqqY0WbIefN+8QfXOIUOYXoIMZITH8AGTwKt ShltJqn2O2yjqllAoG1xLXMRtGAKgKSUO9leyMqad71CWIYR3aMb9V/lvUGSr01DqeYwRecV4Oe1c GI9g3BaW5LpWjdTKQYWQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1p0m5t-008Rfb-Vm; Thu, 01 Dec 2022 16:07:26 +0000 Received: from ams.source.kernel.org ([2604:1380:4601:e00::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1p0m5q-008Rdy-19 for linux-arm-kernel@lists.infradead.org; Thu, 01 Dec 2022 16:07:23 +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 949EBB81E80; Thu, 1 Dec 2022 16:07:18 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id BAF43C433D6; Thu, 1 Dec 2022 16:07:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1669910837; bh=tZk2bauAlJiGxbfhv2iiA5QGYnECZTSJnPppqEnoEsE=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=L9tDT/iQ6KSzpBu6Ar2chF5hHw/xM2wKTE2P6Rp96okbw5LKafBKZohDRidby3l9v wUox/ai4Ro1UoQU/QbHJy0deuaQLAUgWRKYSXLvAzXMpOkvGB2KFRUD8mLqjENLrOZ elkpa4aMUF06hKvESnYmUCUwxyTS9Zt7QqJvj77FEAef3+PIgaLOiHeBPkcE0NKx/u fbaktT3wN4c9PH6zHLFSY9NK8agQVTorgAUuYhl7KnysiazTbdGTuv0Apc2QQsHWaU PxgxjYToFaMnuHrDNUZeJpYsryVZL63MTLiz4h4hJJubYhlYajSgXNtLp4WV/QZ4lD wHqBFuayETqvA== Date: Fri, 2 Dec 2022 01:07:13 +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: <20221202010713.15d9c3bf5520b17a3df8e290@kernel.org> In-Reply-To: References: <166990553243.253128.13594802750635478633.stgit@devnote3> <166990556124.253128.2968612748605960211.stgit@devnote3> 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_080722_400261_8E18B6C1 X-CRM114-Status: GOOD ( 45.26 ) 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 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.) > > 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) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel