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 9B624C4708E for ; Thu, 5 Jan 2023 22:48:08 +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=8MihglfqxNiSt2bACKXNZcDSEuycN7FZnD+3Vetk90w=; b=YMFRxiEdOgSwzR RnKB1t10DB6Xu07l6FayzrmiHH8//rUu/hMtUkRVH0i+vnJR9eJIVRJfbGS2lrIq/1Ux4A08bL9qY 225f44sUKPlz/JPB2UjftID3riUuecYSXQ6Fl5xqOgZGxjS/Z5SrqD/2F64z7dXFabhJ4BJPnD347 oLNC+Ui0qJtRp144vsEzcV0oYAHm04vHdlie6MI2SBgqkR58NY6gipAmwS5MKAe2Xpf9LykIWGk2+ UhwwtY9gKRqXVHlqMeNtlJiwBAaoozfJCqruXIl7uBtVKvQdrbOdd738qYyoyPwaGrpUH7EHVYLCd n4o05vc8GKagPOxNTKvQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pDZ0p-00FlET-C2; Thu, 05 Jan 2023 22:47:05 +0000 Received: from desiato.infradead.org ([2001:8b0:10b:1:d65d:64ff:fe57:4e05]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pDVNF-00Drhm-VK for linux-arm-kernel@bombadil.infradead.org; Thu, 05 Jan 2023 18:53:58 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=desiato.20200630; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=f92ThFDvzxgYEaVZXbu2QzHhVIIE5SYf6ANqIbVIimQ=; b=adFL4eCqPXbKD/uwmg38oqpA/v tG6sbYnTJqIIvxvr7y8wtAKnC5yc8jXnjgeyKBuACz0rFtnYWBH9eyxhgOwYo3prcjPnEFtU00mYu V52/1X7J9PEXKrrDJr87SxxxvPydtqu42+CK39A0MKRhnDd5kY1qQY6Jv3iKQpTclHJuroiv9PTfJ id6teC9Fmwu+U5QWRlWYuWJdiE44ZblTWiOTDUBjWy+E8POACKfOYw0GJ5DrVYjet5PKFP4VJHoS2 ZsuRaTHgssCAE7479TF05+EMeMkP0nvoaCIZGsSIEyatxnfz4u2EgnsnjwYtdP08jykGmoCXCx5k+ OIWhzJaQ==; Received: from foss.arm.com ([217.140.110.172]) by desiato.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pDPd0-001JmH-2z for linux-arm-kernel@lists.infradead.org; Thu, 05 Jan 2023 12:45:53 +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 436F515BF; Thu, 5 Jan 2023 04:46:33 -0800 (PST) Received: from FVFF77S0Q05N (unknown [10.57.45.56]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 812233F663; Thu, 5 Jan 2023 04:45:50 -0800 (PST) Date: Thu, 5 Jan 2023 12:45:44 +0000 From: Mark Rutland To: Ard Biesheuvel Cc: linux-arm-kernel@lists.infradead.org, linux-efi@vger.kernel.org, catalin.marinas@arm.com, will@kernel.org Subject: Re: [PATCH v2 1/2] arm64: efi: Avoid workqueue to check whether EFI runtime is live Message-ID: References: <20230104174433.1259428-1-ardb@kernel.org> <20230104174433.1259428-2-ardb@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230104174433.1259428-2-ardb@kernel.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230105_124551_344610_4459E104 X-CRM114-Status: GOOD ( 26.51 ) 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 Hi Ard, On Wed, Jan 04, 2023 at 06:44:32PM +0100, Ard Biesheuvel wrote: > Comparing current_work() against efi_rts_work.work is sufficient to > decide whether current is currently running EFI runtime services code at > any level in its call stack. > > However, there are other potential users of the EFI runtime stack, such > as the ACPI subsystem, which may invoke efi_call_virt_pointer() > directly, and so any sync exceptions occurring in firmware during those > calls are currently misidentified. > > So instead, let's check whether the spinlock is locked, and whether the > stashed value of the thread stack pointer points into current's thread > stack. This can only be the case if current was interrupted while > running EFI runtime code. > > Signed-off-by: Ard Biesheuvel > --- > arch/arm64/include/asm/efi.h | 10 ++++++++++ > arch/arm64/kernel/efi.c | 3 ++- > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h > index 31d13a6001df49c4..aca6dcaa33efbac4 100644 > --- a/arch/arm64/include/asm/efi.h > +++ b/arch/arm64/include/asm/efi.h > @@ -42,14 +42,24 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md); > > #define arch_efi_call_virt_teardown() \ > ({ \ > + efi_rt_stack_top[-1] = 0; \ Is there any reason not to do this in the asm, given all the other setting of this occurs there? I know that'd mean duplicating the writ for both the regular case and the exception handler, but then it'd be clearly associated with the instant we move away from the EFI RT stack. That would also hide this write from KCSAN; itherwise this'll need to be a WRITE_ONCE() to pair with the (not necessariyl) locked read in current_in_efi() below. > spin_unlock(&efi_rt_lock); \ > __efi_fpsimd_end(); \ > efi_virtmap_unload(); \ > }) > > extern spinlock_t efi_rt_lock; > +extern u64 *efi_rt_stack_top; > efi_status_t __efi_rt_asm_wrapper(void *, const char *, ...); > > +/* > + * efi_rt_stack_top[-1] contains the value the stack pointer had before > + * switching to the EFI runtime stack. > + */ > +#define current_in_efi() \ > + (!preemptible() && spin_is_locked(&efi_rt_lock) && \ > + on_task_stack(current, efi_rt_stack_top[-1], 1)) KCSAN is liable to complain about the access to efi_rt_stack_top[-1], since that can race with another thread updating the value, and it's not necessarily single-copy-atomic. It's probably worth making this a READ_ONCE(), even if we move all the writes to asm, to avoid tearing. Aside from those points, this looks good to me. Thanks, Mark. > + > #define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT) > > /* > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c > index fab05de2e12dd5d8..b273900f45668587 100644 > --- a/arch/arm64/kernel/efi.c > +++ b/arch/arm64/kernel/efi.c > @@ -11,6 +11,7 @@ > #include > > #include > +#include > > static bool region_is_misaligned(const efi_memory_desc_t *md) > { > @@ -154,7 +155,7 @@ asmlinkage efi_status_t __efi_rt_asm_recover(void); > bool efi_runtime_fixup_exception(struct pt_regs *regs, const char *msg) > { > /* Check whether the exception occurred while running the firmware */ > - if (current_work() != &efi_rts_work.work || regs->pc >= TASK_SIZE_64) > + if (!current_in_efi() || regs->pc >= TASK_SIZE_64) > return false; > > pr_err(FW_BUG "Unable to handle %s in EFI runtime service\n", msg); > -- > 2.39.0 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel