Thread (5 messages) flat view 5 messages, 2 authors, 2021-12-20

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help