* [PATCH v2] arm64: errata: Fix exec handling in erratum 1418040 workaround
@ 2021-12-16 19:16 D Scott Phillips
2021-12-16 19:52 ` Marc Zyngier
2021-12-17 14:41 ` Catalin Marinas
0 siblings, 2 replies; 6+ messages in thread
From: D Scott Phillips @ 2021-12-16 19:16 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: Marc Zyngier, Will Deacon, Darren Hart, patches
The erratum 1418040 workaround changes vct access trapping when switching
between compat and non-compat threads. The workaround logic assumes that
the hardware vct trapping state matches the previous task's compat-ness.
However, when a non-compat task execs a compat binary or vice versa, the
cntkctl state and task compat-ness get out of sync. Keep the hardware
trapping state in sync with the task personality.
Fixes: d49f7d7376d0 ("arm64: Move handling of erratum 1418040 into C code")
Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
Reviewed-by: Marc Zyngier <maz@kernel.org>
Cc: <stable@vger.kernel.org> # 5.4.x
---
v2: - Use sysreg_clear_set instead of open coding (Marc)
- guard this_cpu_has_cap() check under IS_ENABLED() to avoid tons of
WARN_ON(preemptible()) when built with !CONFIG_ARM64_ERRATUM_1418040
arch/arm64/include/asm/elf.h | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 97932fbf973d..24036b914226 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -160,8 +160,16 @@ typedef struct user_fpsimd_state elf_fpregset_t;
#define SET_PERSONALITY(ex) \
({ \
+ if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) \
+ preempt_disable(); \
clear_thread_flag(TIF_32BIT); \
current->personality &= ~READ_IMPLIES_EXEC; \
+ if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) { \
+ if (this_cpu_has_cap(ARM64_WORKAROUND_1418040)) \
+ sysreg_clear_set(cntkctl_el1, 0, \
+ ARCH_TIMER_USR_VCT_ACCESS_EN); \
+ preempt_enable(); \
+ } \
})
/* update AT_VECTOR_SIZE_ARCH if the number of NEW_AUX_ENT entries changes */
@@ -223,7 +231,16 @@ int compat_elf_check_arch(const struct elf32_hdr *);
*/
#define COMPAT_SET_PERSONALITY(ex) \
({ \
+ if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) \
+ preempt_disable(); \
set_thread_flag(TIF_32BIT); \
+ if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) { \
+ if (this_cpu_has_cap(ARM64_WORKAROUND_1418040)) \
+ sysreg_clear_set(cntkctl_el1, \
+ ARCH_TIMER_USR_VCT_ACCESS_EN, \
+ 0); \
+ preempt_enable(); \
+ } \
})
#ifdef CONFIG_COMPAT_VDSO
#define COMPAT_ARCH_DLINFO \
--
2.31.1
_______________________________________________
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] 6+ messages in thread
* Re: [PATCH v2] arm64: errata: Fix exec handling in erratum 1418040 workaround
2021-12-16 19:16 [PATCH v2] arm64: errata: Fix exec handling in erratum 1418040 workaround D Scott Phillips
@ 2021-12-16 19:52 ` Marc Zyngier
2021-12-17 14:41 ` Catalin Marinas
1 sibling, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2021-12-16 19:52 UTC (permalink / raw)
To: D Scott Phillips; +Cc: linux-arm-kernel, Will Deacon, Darren Hart, patches
On 2021-12-16 19:16, D Scott Phillips wrote:
> The erratum 1418040 workaround changes vct access trapping when
> switching
> between compat and non-compat threads. The workaround logic assumes
> that
> the hardware vct trapping state matches the previous task's
> compat-ness.
> However, when a non-compat task execs a compat binary or vice versa,
> the
> cntkctl state and task compat-ness get out of sync. Keep the hardware
> trapping state in sync with the task personality.
>
> Fixes: d49f7d7376d0 ("arm64: Move handling of erratum 1418040 into C
> code")
> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
> Reviewed-by: Marc Zyngier <maz@kernel.org>
> Cc: <stable@vger.kernel.org> # 5.4.x
> ---
>
> v2: - Use sysreg_clear_set instead of open coding (Marc)
> - guard this_cpu_has_cap() check under IS_ENABLED() to avoid tons
> of
> WARN_ON(preemptible()) when built with
> !CONFIG_ARM64_ERRATUM_1418040
Indeed.
> arch/arm64/include/asm/elf.h | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/arch/arm64/include/asm/elf.h
> b/arch/arm64/include/asm/elf.h
> index 97932fbf973d..24036b914226 100644
> --- a/arch/arm64/include/asm/elf.h
> +++ b/arch/arm64/include/asm/elf.h
> @@ -160,8 +160,16 @@ typedef struct user_fpsimd_state elf_fpregset_t;
>
> #define SET_PERSONALITY(ex) \
> ({ \
> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) \
> + preempt_disable(); \
> clear_thread_flag(TIF_32BIT); \
> current->personality &= ~READ_IMPLIES_EXEC; \
> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) { \
> + if (this_cpu_has_cap(ARM64_WORKAROUND_1418040)) \
Probably better written as:
if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040) &&
this_cpu_has_cap(ARM64_WORKAROUND_1418040))
sysreg_clear_set(...);
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
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] 6+ messages in thread
* Re: [PATCH v2] arm64: errata: Fix exec handling in erratum 1418040 workaround
2021-12-16 19:16 [PATCH v2] arm64: errata: Fix exec handling in erratum 1418040 workaround D Scott Phillips
2021-12-16 19:52 ` Marc Zyngier
@ 2021-12-17 14:41 ` Catalin Marinas
2021-12-17 16:42 ` D Scott Phillips
1 sibling, 1 reply; 6+ messages in thread
From: Catalin Marinas @ 2021-12-17 14:41 UTC (permalink / raw)
To: D Scott Phillips
Cc: linux-arm-kernel, Marc Zyngier, Will Deacon, Darren Hart, patches
On Thu, Dec 16, 2021 at 11:16:18AM -0800, D Scott Phillips wrote:
> diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
> index 97932fbf973d..24036b914226 100644
> --- a/arch/arm64/include/asm/elf.h
> +++ b/arch/arm64/include/asm/elf.h
> @@ -160,8 +160,16 @@ typedef struct user_fpsimd_state elf_fpregset_t;
>
> #define SET_PERSONALITY(ex) \
> ({ \
> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) \
> + preempt_disable(); \
> clear_thread_flag(TIF_32BIT); \
> current->personality &= ~READ_IMPLIES_EXEC; \
> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) { \
> + if (this_cpu_has_cap(ARM64_WORKAROUND_1418040)) \
> + sysreg_clear_set(cntkctl_el1, 0, \
> + ARCH_TIMER_USR_VCT_ACCESS_EN); \
> + preempt_enable(); \
> + } \
> })
>
> /* update AT_VECTOR_SIZE_ARCH if the number of NEW_AUX_ENT entries changes */
> @@ -223,7 +231,16 @@ int compat_elf_check_arch(const struct elf32_hdr *);
> */
> #define COMPAT_SET_PERSONALITY(ex) \
> ({ \
> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) \
> + preempt_disable(); \
> set_thread_flag(TIF_32BIT); \
> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) { \
> + if (this_cpu_has_cap(ARM64_WORKAROUND_1418040)) \
> + sysreg_clear_set(cntkctl_el1, \
> + ARCH_TIMER_USR_VCT_ACCESS_EN, \
> + 0); \
> + preempt_enable(); \
> + } \
I don't particularly like adding more to these macros. There's
arch_setup_new_exec() that gets called after SET_PERSONALITY, so you can
check whether the task is compat or not.
Thanks.
--
Catalin
_______________________________________________
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] 6+ messages in thread
* Re: [PATCH v2] arm64: errata: Fix exec handling in erratum 1418040 workaround
2021-12-17 14:41 ` Catalin Marinas
@ 2021-12-17 16:42 ` D Scott Phillips
2021-12-17 18:38 ` Catalin Marinas
0 siblings, 1 reply; 6+ messages in thread
From: D Scott Phillips @ 2021-12-17 16:42 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-arm-kernel, Marc Zyngier, Will Deacon, Darren Hart, patches
Catalin Marinas <catalin.marinas@arm.com> writes:
> On Thu, Dec 16, 2021 at 11:16:18AM -0800, D Scott Phillips wrote:
>> diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
>> index 97932fbf973d..24036b914226 100644
>> --- a/arch/arm64/include/asm/elf.h
>> +++ b/arch/arm64/include/asm/elf.h
>> @@ -160,8 +160,16 @@ typedef struct user_fpsimd_state elf_fpregset_t;
>>
>> #define SET_PERSONALITY(ex) \
>> ({ \
>> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) \
>> + preempt_disable(); \
>> clear_thread_flag(TIF_32BIT); \
>> current->personality &= ~READ_IMPLIES_EXEC; \
>> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) { \
>> + if (this_cpu_has_cap(ARM64_WORKAROUND_1418040)) \
>> + sysreg_clear_set(cntkctl_el1, 0, \
>> + ARCH_TIMER_USR_VCT_ACCESS_EN); \
>> + preempt_enable(); \
>> + } \
>> })
>>
>> /* update AT_VECTOR_SIZE_ARCH if the number of NEW_AUX_ENT entries changes */
>> @@ -223,7 +231,16 @@ int compat_elf_check_arch(const struct elf32_hdr *);
>> */
>> #define COMPAT_SET_PERSONALITY(ex) \
>> ({ \
>> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) \
>> + preempt_disable(); \
>> set_thread_flag(TIF_32BIT); \
>> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) { \
>> + if (this_cpu_has_cap(ARM64_WORKAROUND_1418040)) \
>> + sysreg_clear_set(cntkctl_el1, \
>> + ARCH_TIMER_USR_VCT_ACCESS_EN, \
>> + 0); \
>> + preempt_enable(); \
>> + } \
>
> I don't particularly like adding more to these macros. There's
> arch_setup_new_exec() that gets called after SET_PERSONALITY, so you can
> check whether the task is compat or not.
If the task is preemptible between the update to TIF_32BIT and the
update to cntkctl then a window exists where preemption will confuse the
workaround logic we have in switch_to, causing the desync between compat
state and counter access trapping to propagate into other tasks.
Agreed that this change gunks up a previously simple macro though. Maybe
we could have SET_PERSONALITY only set a flag in arch_elf_state and
arch_setup_new_exec() take over setting TIF_32BIT? Or move
set_personality's implementation to a helper? Or something else?
> Thanks.
>
> --
> Catalin
_______________________________________________
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] 6+ messages in thread
* Re: [PATCH v2] arm64: errata: Fix exec handling in erratum 1418040 workaround
2021-12-17 16:42 ` D Scott Phillips
@ 2021-12-17 18:38 ` Catalin Marinas
2021-12-17 19:29 ` D Scott Phillips
0 siblings, 1 reply; 6+ messages in thread
From: Catalin Marinas @ 2021-12-17 18:38 UTC (permalink / raw)
To: D Scott Phillips
Cc: linux-arm-kernel, Marc Zyngier, Will Deacon, Darren Hart, patches
On Fri, Dec 17, 2021 at 08:42:01AM -0800, D Scott Phillips wrote:
> Catalin Marinas <catalin.marinas@arm.com> writes:
> > On Thu, Dec 16, 2021 at 11:16:18AM -0800, D Scott Phillips wrote:
> >> diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
> >> index 97932fbf973d..24036b914226 100644
> >> --- a/arch/arm64/include/asm/elf.h
> >> +++ b/arch/arm64/include/asm/elf.h
> >> @@ -160,8 +160,16 @@ typedef struct user_fpsimd_state elf_fpregset_t;
> >>
> >> #define SET_PERSONALITY(ex) \
> >> ({ \
> >> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) \
> >> + preempt_disable(); \
> >> clear_thread_flag(TIF_32BIT); \
> >> current->personality &= ~READ_IMPLIES_EXEC; \
> >> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) { \
> >> + if (this_cpu_has_cap(ARM64_WORKAROUND_1418040)) \
> >> + sysreg_clear_set(cntkctl_el1, 0, \
> >> + ARCH_TIMER_USR_VCT_ACCESS_EN); \
> >> + preempt_enable(); \
> >> + } \
> >> })
> >>
> >> /* update AT_VECTOR_SIZE_ARCH if the number of NEW_AUX_ENT entries changes */
> >> @@ -223,7 +231,16 @@ int compat_elf_check_arch(const struct elf32_hdr *);
> >> */
> >> #define COMPAT_SET_PERSONALITY(ex) \
> >> ({ \
> >> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) \
> >> + preempt_disable(); \
> >> set_thread_flag(TIF_32BIT); \
> >> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) { \
> >> + if (this_cpu_has_cap(ARM64_WORKAROUND_1418040)) \
> >> + sysreg_clear_set(cntkctl_el1, \
> >> + ARCH_TIMER_USR_VCT_ACCESS_EN, \
> >> + 0); \
> >> + preempt_enable(); \
> >> + } \
> >
> > I don't particularly like adding more to these macros. There's
> > arch_setup_new_exec() that gets called after SET_PERSONALITY, so you can
> > check whether the task is compat or not.
>
> If the task is preemptible between the update to TIF_32BIT and the
> update to cntkctl then a window exists where preemption will confuse the
> workaround logic we have in switch_to, causing the desync between compat
> state and counter access trapping to propagate into other tasks.
>
> Agreed that this change gunks up a previously simple macro though. Maybe
> we could have SET_PERSONALITY only set a flag in arch_elf_state and
> arch_setup_new_exec() take over setting TIF_32BIT? Or move
> set_personality's implementation to a helper? Or something else?
Is reading ckntkctl_el1 as expensive as the write? If not we could
change the __switch_to() logic to simply check that the bit is set or
cleared depending on is_compat_thread() and skip the comparison with the
previous task.
--
Catalin
_______________________________________________
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] 6+ messages in thread
* Re: [PATCH v2] arm64: errata: Fix exec handling in erratum 1418040 workaround
2021-12-17 18:38 ` Catalin Marinas
@ 2021-12-17 19:29 ` D Scott Phillips
0 siblings, 0 replies; 6+ messages in thread
From: D Scott Phillips @ 2021-12-17 19:29 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-arm-kernel, Marc Zyngier, Will Deacon, Darren Hart, patches
Catalin Marinas <catalin.marinas@arm.com> writes:
> On Fri, Dec 17, 2021 at 08:42:01AM -0800, D Scott Phillips wrote:
>> Catalin Marinas <catalin.marinas@arm.com> writes:
>> > On Thu, Dec 16, 2021 at 11:16:18AM -0800, D Scott Phillips wrote:
>> >> diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
>> >> index 97932fbf973d..24036b914226 100644
>> >> --- a/arch/arm64/include/asm/elf.h
>> >> +++ b/arch/arm64/include/asm/elf.h
>> >> @@ -160,8 +160,16 @@ typedef struct user_fpsimd_state elf_fpregset_t;
>> >>
>> >> #define SET_PERSONALITY(ex) \
>> >> ({ \
>> >> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) \
>> >> + preempt_disable(); \
>> >> clear_thread_flag(TIF_32BIT); \
>> >> current->personality &= ~READ_IMPLIES_EXEC; \
>> >> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) { \
>> >> + if (this_cpu_has_cap(ARM64_WORKAROUND_1418040)) \
>> >> + sysreg_clear_set(cntkctl_el1, 0, \
>> >> + ARCH_TIMER_USR_VCT_ACCESS_EN); \
>> >> + preempt_enable(); \
>> >> + } \
>> >> })
>> >>
>> >> /* update AT_VECTOR_SIZE_ARCH if the number of NEW_AUX_ENT entries changes */
>> >> @@ -223,7 +231,16 @@ int compat_elf_check_arch(const struct elf32_hdr *);
>> >> */
>> >> #define COMPAT_SET_PERSONALITY(ex) \
>> >> ({ \
>> >> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) \
>> >> + preempt_disable(); \
>> >> set_thread_flag(TIF_32BIT); \
>> >> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) { \
>> >> + if (this_cpu_has_cap(ARM64_WORKAROUND_1418040)) \
>> >> + sysreg_clear_set(cntkctl_el1, \
>> >> + ARCH_TIMER_USR_VCT_ACCESS_EN, \
>> >> + 0); \
>> >> + preempt_enable(); \
>> >> + } \
>> >
>> > I don't particularly like adding more to these macros. There's
>> > arch_setup_new_exec() that gets called after SET_PERSONALITY, so you can
>> > check whether the task is compat or not.
>>
>> If the task is preemptible between the update to TIF_32BIT and the
>> update to cntkctl then a window exists where preemption will confuse the
>> workaround logic we have in switch_to, causing the desync between compat
>> state and counter access trapping to propagate into other tasks.
>>
>> Agreed that this change gunks up a previously simple macro though. Maybe
>> we could have SET_PERSONALITY only set a flag in arch_elf_state and
>> arch_setup_new_exec() take over setting TIF_32BIT? Or move
>> set_personality's implementation to a helper? Or something else?
>
> Is reading ckntkctl_el1 as expensive as the write? If not we could
> change the __switch_to() logic to simply check that the bit is set or
> cleared depending on is_compat_thread() and skip the comparison with the
> previous task.
Ah, yes that's a good idea. From a quick check on an N1 core, it looks
like reading cntkctl has about the same time cost as reading a general
purpose register. I'd assume the other cores affected by the erratum
would be similar, so I'll go that route. Thanks,
Scott
> --
> Catalin
_______________________________________________
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] 6+ messages in thread
end of thread, other threads:[~2021-12-17 19:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-16 19:16 [PATCH v2] arm64: errata: Fix exec handling in erratum 1418040 workaround D Scott Phillips
2021-12-16 19:52 ` Marc Zyngier
2021-12-17 14:41 ` Catalin Marinas
2021-12-17 16:42 ` D Scott Phillips
2021-12-17 18:38 ` Catalin Marinas
2021-12-17 19:29 ` D Scott Phillips
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).