Re: [PATCH v4] arm64: errata: Fix exec handling in erratum 1418040 workaround
From: Marc Zyngier <maz@kernel.org>
Date: 2021-12-20 12:05:05
On Fri, 17 Dec 2021 21:19:20 +0000, D Scott Phillips [off-list ref] wrote:
The erratum 1418040 workaround enables vct access trapping when executing
nit: s/vct/CNTVCT_EL1/.
quoted hunk ↗ jump to hunk
compat threads. The workaround is applied when switching between tasks, but the need for the workaround could also change at an exec(), when a non-compat task execs a compat binary or vice versa. Apply the workaround in arch_setup_new_exec(). The leaves a small window of time between SET_PERSONALITY and arch_setup_new_exec where preemption could occur and confuse the old workaround logic that compares TIF_32BIT between prev and next. Instead, we can just read cntkctl to make sure it's in the state that the next task needs. I measured cntkctl read time to be about the same as a mov from a general-purpose register on N1. Update the workaround logic to examine the current value of cntkctl instead of the previous task's compat state. Fixes: d49f7d7376d0 ("arm64: Move handling of erratum 1418040 into C code") Signed-off-by: D Scott Phillips <redacted> Cc: <redacted> # 5.4.x --- v4: - Move exec() handling into arch_setup_new_exec(), drop prev32==next32 comparison to fix possible confusion in the small window between SET_PERSONALITY() and arch_setup_new_exec(). (Catalin) v3: - Un-nest conditionals (Marc) 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/kernel/process.c | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-)diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index aacf2f5559a8..b37ff23e625e 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c@@ -439,34 +439,23 @@ static void entry_task_switch(struct task_struct *next) /* * ARM erratum 1418040 handling, affecting the 32bit view of CNTVCT. - * Assuming the virtual counter is enabled at the beginning of times: - * - * - disable access when switching from a 64bit task to a 32bit task - * - enable access when switching from a 32bit task to a 64bit task + * Ensure access is disabled when switching to a 32bit task, ensure + * access is enabled when switching to a 64bit task. */ -static void erratum_1418040_thread_switch(struct task_struct *prev, - struct task_struct *next) +static void erratum_1418040_thread_switch(struct task_struct *next) { - bool prev32, next32; - u64 val; - - if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) - return; + preempt_disable();
I'd rather avoid this on the __switch_to() path. We're guaranteed to be non-preemptible when called from there, and we want it to be as fast as possible. It would also avoid the bug on the early return just below.
quoted hunk ↗ jump to hunk
- prev32 = is_compat_thread(task_thread_info(prev)); - next32 = is_compat_thread(task_thread_info(next)); - - if (prev32 == next32 || !this_cpu_has_cap(ARM64_WORKAROUND_1418040)) + if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040) || + !this_cpu_has_cap(ARM64_WORKAROUND_1418040)) return; - val = read_sysreg(cntkctl_el1); - - if (!next32) - val |= ARCH_TIMER_USR_VCT_ACCESS_EN; + if (is_compat_thread(task_thread_info(next))) + sysreg_clear_set(cntkctl_el1, ARCH_TIMER_USR_VCT_ACCESS_EN, 0); else - val &= ~ARCH_TIMER_USR_VCT_ACCESS_EN; + sysreg_clear_set(cntkctl_el1, 0, ARCH_TIMER_USR_VCT_ACCESS_EN); - write_sysreg(val, cntkctl_el1); + preempt_enable(); } /*@@ -501,7 +490,7 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev, contextidr_thread_switch(next); entry_task_switch(next); ssbs_thread_switch(next); - erratum_1418040_thread_switch(prev, next); + erratum_1418040_thread_switch(next); ptrauth_thread_switch_user(next); /*@@ -611,6 +600,7 @@ void arch_setup_new_exec(void) current->mm->context.flags = mmflags; ptrauth_thread_init_user(); mte_thread_init_user(); + erratum_1418040_thread_switch(current);
But what is the point of this now? As you enter __switch_to(), the TIF flags are set in stone for this particular return to userspace. Since you are now evaluating the state of CNTKCTL_EL1 on each and every switch, you are guaranteed to set the enable bit to the right value on each return to userspace, even if you have gone via SET_PERSONALITY(). Am I missing something? Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel