* [PATCH v2 0/2] efi: Follow-up fixes for EFI runtime stack
@ 2023-01-04 17:44 ` Ard Biesheuvel
0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2023-01-04 17:44 UTC (permalink / raw)
To: linux-arm-kernel
Cc: linux-efi, catalin.marinas, will, Ard Biesheuvel, Mark Rutland,
Lee Jones
Commit ff7a167961d1b ("arm64: efi: Execute runtime services from a
dedicated stack") introduced a dedicated stack for EFI runtime services,
in an attempt to make the execution of EFI runtime services more robust,
given that they execute at the same privilege level as the kernel.
However, this stack needs to be declared to the stacktrace machinery,
which is careful not to walk the stack when it leads into memory regions
that are not known to be allocated for stack use.
Also, given that the ACPI code may invoke the low-level EFI runtime call
wrapper without using the dedicated kernel thread and workqueue, we
should take this into account when trying to gracefully handle
synchronous exceptions.
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lee Jones <lee@kernel.org>
Ard Biesheuvel (2):
arm64: efi: Avoid workqueue to check whether EFI runtime is live
arm64: efi: Account for the EFI runtime stack in stack unwinder
arch/arm64/include/asm/efi.h | 10 ++++++++++
arch/arm64/include/asm/stacktrace.h | 15 +++++++++++++++
arch/arm64/kernel/efi.c | 3 ++-
arch/arm64/kernel/stacktrace.c | 12 ++++++++++++
4 files changed, 39 insertions(+), 1 deletion(-)
--
2.39.0
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 0/2] efi: Follow-up fixes for EFI runtime stack @ 2023-01-04 17:44 ` Ard Biesheuvel 0 siblings, 0 replies; 12+ messages in thread From: Ard Biesheuvel @ 2023-01-04 17:44 UTC (permalink / raw) To: linux-arm-kernel Cc: linux-efi, catalin.marinas, will, Ard Biesheuvel, Mark Rutland, Lee Jones Commit ff7a167961d1b ("arm64: efi: Execute runtime services from a dedicated stack") introduced a dedicated stack for EFI runtime services, in an attempt to make the execution of EFI runtime services more robust, given that they execute at the same privilege level as the kernel. However, this stack needs to be declared to the stacktrace machinery, which is careful not to walk the stack when it leads into memory regions that are not known to be allocated for stack use. Also, given that the ACPI code may invoke the low-level EFI runtime call wrapper without using the dedicated kernel thread and workqueue, we should take this into account when trying to gracefully handle synchronous exceptions. Cc: Mark Rutland <mark.rutland@arm.com> Cc: Lee Jones <lee@kernel.org> Ard Biesheuvel (2): arm64: efi: Avoid workqueue to check whether EFI runtime is live arm64: efi: Account for the EFI runtime stack in stack unwinder arch/arm64/include/asm/efi.h | 10 ++++++++++ arch/arm64/include/asm/stacktrace.h | 15 +++++++++++++++ arch/arm64/kernel/efi.c | 3 ++- arch/arm64/kernel/stacktrace.c | 12 ++++++++++++ 4 files changed, 39 insertions(+), 1 deletion(-) -- 2.39.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] arm64: efi: Avoid workqueue to check whether EFI runtime is live 2023-01-04 17:44 ` Ard Biesheuvel @ 2023-01-04 17:44 ` Ard Biesheuvel -1 siblings, 0 replies; 12+ messages in thread From: Ard Biesheuvel @ 2023-01-04 17:44 UTC (permalink / raw) To: linux-arm-kernel; +Cc: linux-efi, catalin.marinas, will, Ard Biesheuvel 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 <ardb@kernel.org> --- 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; \ 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)) + #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 <linux/init.h> #include <asm/efi.h> +#include <asm/stacktrace.h> 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] arm64: efi: Avoid workqueue to check whether EFI runtime is live @ 2023-01-04 17:44 ` Ard Biesheuvel 0 siblings, 0 replies; 12+ messages in thread From: Ard Biesheuvel @ 2023-01-04 17:44 UTC (permalink / raw) To: linux-arm-kernel; +Cc: linux-efi, catalin.marinas, will, Ard Biesheuvel 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 <ardb@kernel.org> --- 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; \ 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)) + #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 <linux/init.h> #include <asm/efi.h> +#include <asm/stacktrace.h> 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] arm64: efi: Avoid workqueue to check whether EFI runtime is live 2023-01-04 17:44 ` Ard Biesheuvel @ 2023-01-05 12:45 ` Mark Rutland -1 siblings, 0 replies; 12+ messages in thread From: Mark Rutland @ 2023-01-05 12:45 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: linux-arm-kernel, linux-efi, catalin.marinas, will 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 <ardb@kernel.org> > --- > 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 <linux/init.h> > > #include <asm/efi.h> > +#include <asm/stacktrace.h> > > 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] arm64: efi: Avoid workqueue to check whether EFI runtime is live @ 2023-01-05 12:45 ` Mark Rutland 0 siblings, 0 replies; 12+ messages in thread From: Mark Rutland @ 2023-01-05 12:45 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: linux-arm-kernel, linux-efi, catalin.marinas, will 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 <ardb@kernel.org> > --- > 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 <linux/init.h> > > #include <asm/efi.h> > +#include <asm/stacktrace.h> > > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] arm64: efi: Avoid workqueue to check whether EFI runtime is live 2023-01-05 12:45 ` Mark Rutland @ 2023-01-06 15:42 ` Ard Biesheuvel -1 siblings, 0 replies; 12+ messages in thread From: Ard Biesheuvel @ 2023-01-06 15:42 UTC (permalink / raw) To: Mark Rutland; +Cc: linux-arm-kernel, linux-efi, catalin.marinas, will On Thu, 5 Jan 2023 at 13:46, Mark Rutland <mark.rutland@arm.com> wrote: > > 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 <ardb@kernel.org> > > --- > > 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. > Sure. > > 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. > ok ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] arm64: efi: Avoid workqueue to check whether EFI runtime is live @ 2023-01-06 15:42 ` Ard Biesheuvel 0 siblings, 0 replies; 12+ messages in thread From: Ard Biesheuvel @ 2023-01-06 15:42 UTC (permalink / raw) To: Mark Rutland; +Cc: linux-arm-kernel, linux-efi, catalin.marinas, will On Thu, 5 Jan 2023 at 13:46, Mark Rutland <mark.rutland@arm.com> wrote: > > 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 <ardb@kernel.org> > > --- > > 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. > Sure. > > 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. > ok _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] arm64: efi: Account for the EFI runtime stack in stack unwinder 2023-01-04 17:44 ` Ard Biesheuvel @ 2023-01-04 17:44 ` Ard Biesheuvel -1 siblings, 0 replies; 12+ messages in thread From: Ard Biesheuvel @ 2023-01-04 17:44 UTC (permalink / raw) To: linux-arm-kernel; +Cc: linux-efi, catalin.marinas, will, Ard Biesheuvel The EFI runtime services run from a dedicated stack now, and so the stack unwinder needs to be informed about this. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/arm64/include/asm/stacktrace.h | 15 +++++++++++++++ arch/arm64/kernel/stacktrace.c | 12 ++++++++++++ 2 files changed, 27 insertions(+) diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h index 4e5354beafb01bac..66ec8caa6ac07fa0 100644 --- a/arch/arm64/include/asm/stacktrace.h +++ b/arch/arm64/include/asm/stacktrace.h @@ -106,4 +106,19 @@ static inline struct stack_info stackinfo_get_sdei_critical(void) #define stackinfo_get_sdei_critical() stackinfo_get_unknown() #endif +#ifdef CONFIG_EFI +extern u64 *efi_rt_stack_top; + +static inline struct stack_info stackinfo_get_efi(void) +{ + unsigned long high = (u64)efi_rt_stack_top; + unsigned long low = high - THREAD_SIZE; + + return (struct stack_info) { + .low = low, + .high = high, + }; +} +#endif + #endif /* __ASM_STACKTRACE_H */ diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index 117e2c180f3c77d8..83154303e682c8b6 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -5,6 +5,7 @@ * Copyright (C) 2012 ARM Ltd. */ #include <linux/kernel.h> +#include <linux/efi.h> #include <linux/export.h> #include <linux/ftrace.h> #include <linux/sched.h> @@ -12,6 +13,7 @@ #include <linux/sched/task_stack.h> #include <linux/stacktrace.h> +#include <asm/efi.h> #include <asm/irq.h> #include <asm/stack_pointer.h> #include <asm/stacktrace.h> @@ -186,6 +188,13 @@ void show_stack(struct task_struct *tsk, unsigned long *sp, const char *loglvl) : stackinfo_get_unknown(); \ }) +#define STACKINFO_EFI \ + ({ \ + ((task == current) && current_in_efi()) \ + ? stackinfo_get_efi() \ + : stackinfo_get_unknown(); \ + }) + noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, struct task_struct *task, struct pt_regs *regs) @@ -199,6 +208,9 @@ noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry, #if defined(CONFIG_VMAP_STACK) && defined(CONFIG_ARM_SDE_INTERFACE) STACKINFO_SDEI(normal), STACKINFO_SDEI(critical), +#endif +#ifdef CONFIG_EFI + STACKINFO_EFI, #endif }; struct unwind_state state = { -- 2.39.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] arm64: efi: Account for the EFI runtime stack in stack unwinder @ 2023-01-04 17:44 ` Ard Biesheuvel 0 siblings, 0 replies; 12+ messages in thread From: Ard Biesheuvel @ 2023-01-04 17:44 UTC (permalink / raw) To: linux-arm-kernel; +Cc: linux-efi, catalin.marinas, will, Ard Biesheuvel The EFI runtime services run from a dedicated stack now, and so the stack unwinder needs to be informed about this. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/arm64/include/asm/stacktrace.h | 15 +++++++++++++++ arch/arm64/kernel/stacktrace.c | 12 ++++++++++++ 2 files changed, 27 insertions(+) diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h index 4e5354beafb01bac..66ec8caa6ac07fa0 100644 --- a/arch/arm64/include/asm/stacktrace.h +++ b/arch/arm64/include/asm/stacktrace.h @@ -106,4 +106,19 @@ static inline struct stack_info stackinfo_get_sdei_critical(void) #define stackinfo_get_sdei_critical() stackinfo_get_unknown() #endif +#ifdef CONFIG_EFI +extern u64 *efi_rt_stack_top; + +static inline struct stack_info stackinfo_get_efi(void) +{ + unsigned long high = (u64)efi_rt_stack_top; + unsigned long low = high - THREAD_SIZE; + + return (struct stack_info) { + .low = low, + .high = high, + }; +} +#endif + #endif /* __ASM_STACKTRACE_H */ diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index 117e2c180f3c77d8..83154303e682c8b6 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -5,6 +5,7 @@ * Copyright (C) 2012 ARM Ltd. */ #include <linux/kernel.h> +#include <linux/efi.h> #include <linux/export.h> #include <linux/ftrace.h> #include <linux/sched.h> @@ -12,6 +13,7 @@ #include <linux/sched/task_stack.h> #include <linux/stacktrace.h> +#include <asm/efi.h> #include <asm/irq.h> #include <asm/stack_pointer.h> #include <asm/stacktrace.h> @@ -186,6 +188,13 @@ void show_stack(struct task_struct *tsk, unsigned long *sp, const char *loglvl) : stackinfo_get_unknown(); \ }) +#define STACKINFO_EFI \ + ({ \ + ((task == current) && current_in_efi()) \ + ? stackinfo_get_efi() \ + : stackinfo_get_unknown(); \ + }) + noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, struct task_struct *task, struct pt_regs *regs) @@ -199,6 +208,9 @@ noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry, #if defined(CONFIG_VMAP_STACK) && defined(CONFIG_ARM_SDE_INTERFACE) STACKINFO_SDEI(normal), STACKINFO_SDEI(critical), +#endif +#ifdef CONFIG_EFI + STACKINFO_EFI, #endif }; struct unwind_state state = { -- 2.39.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] arm64: efi: Account for the EFI runtime stack in stack unwinder 2023-01-04 17:44 ` Ard Biesheuvel @ 2023-01-05 12:47 ` Mark Rutland -1 siblings, 0 replies; 12+ messages in thread From: Mark Rutland @ 2023-01-05 12:47 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: linux-arm-kernel, linux-efi, catalin.marinas, will On Wed, Jan 04, 2023 at 06:44:33PM +0100, Ard Biesheuvel wrote: > The EFI runtime services run from a dedicated stack now, and so the > stack unwinder needs to be informed about this. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks for this! Mark. > --- > arch/arm64/include/asm/stacktrace.h | 15 +++++++++++++++ > arch/arm64/kernel/stacktrace.c | 12 ++++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h > index 4e5354beafb01bac..66ec8caa6ac07fa0 100644 > --- a/arch/arm64/include/asm/stacktrace.h > +++ b/arch/arm64/include/asm/stacktrace.h > @@ -106,4 +106,19 @@ static inline struct stack_info stackinfo_get_sdei_critical(void) > #define stackinfo_get_sdei_critical() stackinfo_get_unknown() > #endif > > +#ifdef CONFIG_EFI > +extern u64 *efi_rt_stack_top; > + > +static inline struct stack_info stackinfo_get_efi(void) > +{ > + unsigned long high = (u64)efi_rt_stack_top; > + unsigned long low = high - THREAD_SIZE; > + > + return (struct stack_info) { > + .low = low, > + .high = high, > + }; > +} > +#endif > + > #endif /* __ASM_STACKTRACE_H */ > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index 117e2c180f3c77d8..83154303e682c8b6 100644 > --- a/arch/arm64/kernel/stacktrace.c > +++ b/arch/arm64/kernel/stacktrace.c > @@ -5,6 +5,7 @@ > * Copyright (C) 2012 ARM Ltd. > */ > #include <linux/kernel.h> > +#include <linux/efi.h> > #include <linux/export.h> > #include <linux/ftrace.h> > #include <linux/sched.h> > @@ -12,6 +13,7 @@ > #include <linux/sched/task_stack.h> > #include <linux/stacktrace.h> > > +#include <asm/efi.h> > #include <asm/irq.h> > #include <asm/stack_pointer.h> > #include <asm/stacktrace.h> > @@ -186,6 +188,13 @@ void show_stack(struct task_struct *tsk, unsigned long *sp, const char *loglvl) > : stackinfo_get_unknown(); \ > }) > > +#define STACKINFO_EFI \ > + ({ \ > + ((task == current) && current_in_efi()) \ > + ? stackinfo_get_efi() \ > + : stackinfo_get_unknown(); \ > + }) > + > noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry, > void *cookie, struct task_struct *task, > struct pt_regs *regs) > @@ -199,6 +208,9 @@ noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry, > #if defined(CONFIG_VMAP_STACK) && defined(CONFIG_ARM_SDE_INTERFACE) > STACKINFO_SDEI(normal), > STACKINFO_SDEI(critical), > +#endif > +#ifdef CONFIG_EFI > + STACKINFO_EFI, > #endif > }; > struct unwind_state state = { > -- > 2.39.0 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] arm64: efi: Account for the EFI runtime stack in stack unwinder @ 2023-01-05 12:47 ` Mark Rutland 0 siblings, 0 replies; 12+ messages in thread From: Mark Rutland @ 2023-01-05 12:47 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: linux-arm-kernel, linux-efi, catalin.marinas, will On Wed, Jan 04, 2023 at 06:44:33PM +0100, Ard Biesheuvel wrote: > The EFI runtime services run from a dedicated stack now, and so the > stack unwinder needs to be informed about this. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks for this! Mark. > --- > arch/arm64/include/asm/stacktrace.h | 15 +++++++++++++++ > arch/arm64/kernel/stacktrace.c | 12 ++++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h > index 4e5354beafb01bac..66ec8caa6ac07fa0 100644 > --- a/arch/arm64/include/asm/stacktrace.h > +++ b/arch/arm64/include/asm/stacktrace.h > @@ -106,4 +106,19 @@ static inline struct stack_info stackinfo_get_sdei_critical(void) > #define stackinfo_get_sdei_critical() stackinfo_get_unknown() > #endif > > +#ifdef CONFIG_EFI > +extern u64 *efi_rt_stack_top; > + > +static inline struct stack_info stackinfo_get_efi(void) > +{ > + unsigned long high = (u64)efi_rt_stack_top; > + unsigned long low = high - THREAD_SIZE; > + > + return (struct stack_info) { > + .low = low, > + .high = high, > + }; > +} > +#endif > + > #endif /* __ASM_STACKTRACE_H */ > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index 117e2c180f3c77d8..83154303e682c8b6 100644 > --- a/arch/arm64/kernel/stacktrace.c > +++ b/arch/arm64/kernel/stacktrace.c > @@ -5,6 +5,7 @@ > * Copyright (C) 2012 ARM Ltd. > */ > #include <linux/kernel.h> > +#include <linux/efi.h> > #include <linux/export.h> > #include <linux/ftrace.h> > #include <linux/sched.h> > @@ -12,6 +13,7 @@ > #include <linux/sched/task_stack.h> > #include <linux/stacktrace.h> > > +#include <asm/efi.h> > #include <asm/irq.h> > #include <asm/stack_pointer.h> > #include <asm/stacktrace.h> > @@ -186,6 +188,13 @@ void show_stack(struct task_struct *tsk, unsigned long *sp, const char *loglvl) > : stackinfo_get_unknown(); \ > }) > > +#define STACKINFO_EFI \ > + ({ \ > + ((task == current) && current_in_efi()) \ > + ? stackinfo_get_efi() \ > + : stackinfo_get_unknown(); \ > + }) > + > noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry, > void *cookie, struct task_struct *task, > struct pt_regs *regs) > @@ -199,6 +208,9 @@ noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry, > #if defined(CONFIG_VMAP_STACK) && defined(CONFIG_ARM_SDE_INTERFACE) > STACKINFO_SDEI(normal), > STACKINFO_SDEI(critical), > +#endif > +#ifdef CONFIG_EFI > + STACKINFO_EFI, > #endif > }; > struct unwind_state state = { > -- > 2.39.0 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-01-06 15:43 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-04 17:44 [PATCH v2 0/2] efi: Follow-up fixes for EFI runtime stack Ard Biesheuvel 2023-01-04 17:44 ` Ard Biesheuvel 2023-01-04 17:44 ` [PATCH v2 1/2] arm64: efi: Avoid workqueue to check whether EFI runtime is live Ard Biesheuvel 2023-01-04 17:44 ` Ard Biesheuvel 2023-01-05 12:45 ` Mark Rutland 2023-01-05 12:45 ` Mark Rutland 2023-01-06 15:42 ` Ard Biesheuvel 2023-01-06 15:42 ` Ard Biesheuvel 2023-01-04 17:44 ` [PATCH v2 2/2] arm64: efi: Account for the EFI runtime stack in stack unwinder Ard Biesheuvel 2023-01-04 17:44 ` Ard Biesheuvel 2023-01-05 12:47 ` Mark Rutland 2023-01-05 12:47 ` Mark Rutland
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.