All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4] arm64: errata: Fix exec handling in erratum 1418040 workaround
@ 2021-12-17 21:19 D Scott Phillips
  2021-12-17 21:36 ` D Scott Phillips
  2021-12-20 12:03 ` Marc Zyngier
  0 siblings, 2 replies; 5+ messages in thread
From: D Scott Phillips @ 2021-12-17 21:19 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Catalin Marinas, Marc Zyngier, Will Deacon, Darren Hart, patches

The erratum 1418040 workaround enables vct access trapping when executing
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 <scott@os.amperecomputing.com>
Cc: <stable@vger.kernel.org> # 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();
 
-	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);
 
 	if (task_spec_ssb_noexec(current)) {
 		arch_prctl_spec_ctrl_set(current, PR_SPEC_STORE_BYPASS,
-- 
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] 5+ messages in thread

* Re: [PATCH v4] arm64: errata: Fix exec handling in erratum 1418040 workaround
  2021-12-17 21:19 [PATCH v4] arm64: errata: Fix exec handling in erratum 1418040 workaround D Scott Phillips
@ 2021-12-17 21:36 ` D Scott Phillips
  2021-12-20 12:03 ` Marc Zyngier
  1 sibling, 0 replies; 5+ messages in thread
From: D Scott Phillips @ 2021-12-17 21:36 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Catalin Marinas, Marc Zyngier, Will Deacon, Darren Hart, patches

D Scott Phillips <scott@os.amperecomputing.com> writes:

> 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();
>  
> -	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;

Oops, missing preempt_enable() here.

_______________________________________________
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] 5+ messages in thread

* Re: [PATCH v4] arm64: errata: Fix exec handling in erratum 1418040 workaround
  2021-12-17 21:19 [PATCH v4] arm64: errata: Fix exec handling in erratum 1418040 workaround D Scott Phillips
  2021-12-17 21:36 ` D Scott Phillips
@ 2021-12-20 12:03 ` Marc Zyngier
  2021-12-20 16:40   ` D Scott Phillips
  1 sibling, 1 reply; 5+ messages in thread
From: Marc Zyngier @ 2021-12-20 12:03 UTC (permalink / raw)
  To: D Scott Phillips
  Cc: linux-arm-kernel, Catalin Marinas, Will Deacon, Darren Hart, patches

On Fri, 17 Dec 2021 21:19:20 +0000,
D Scott Phillips <scott@os.amperecomputing.com> wrote:
> 
> The erratum 1418040 workaround enables vct access trapping when executing

nit: s/vct/CNTVCT_EL1/.

> 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 <scott@os.amperecomputing.com>
> Cc: <stable@vger.kernel.org> # 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.

>
> -	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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v4] arm64: errata: Fix exec handling in erratum 1418040 workaround
  2021-12-20 12:03 ` Marc Zyngier
@ 2021-12-20 16:40   ` D Scott Phillips
  2021-12-20 17:36     ` Marc Zyngier
  0 siblings, 1 reply; 5+ messages in thread
From: D Scott Phillips @ 2021-12-20 16:40 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: linux-arm-kernel, Catalin Marinas, Will Deacon, Darren Hart, patches

Marc Zyngier <maz@kernel.org> writes:

> On Fri, 17 Dec 2021 21:19:20 +0000,
> D Scott Phillips <scott@os.amperecomputing.com> wrote:
>> 
>> The erratum 1418040 workaround enables vct access trapping when executing
>
> nit: s/vct/CNTVCT_EL1/.

fixed, thanks

>> 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 <scott@os.amperecomputing.com>
>> Cc: <stable@vger.kernel.org> # 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.

Yes, makes sense. I've added an erratum_1418040_new_exec() helper that
does preempt_disable/erratum_14518040_thread_switch(current)/preempt_enable,
and called it from arch_setup_new_exec().

>>
>> -	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?

The workaround in __switch_to isn't happening for every return to
userspace, but rather for every scheduler task switch. When a process
exec()s, no switch happens at that point. From the scheduler point of
view, this is still the same task.  So in the time period between
exec()ing a compat task from non-compat (or vice versa) and the first
time it gets switched out, we would apply the wrong workaround state,
unless we make a change to cntkctl from exec() before returning back to
EL0.

Thanks,

Scott

_______________________________________________
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] 5+ messages in thread

* Re: [PATCH v4] arm64: errata: Fix exec handling in erratum 1418040 workaround
  2021-12-20 16:40   ` D Scott Phillips
@ 2021-12-20 17:36     ` Marc Zyngier
  0 siblings, 0 replies; 5+ messages in thread
From: Marc Zyngier @ 2021-12-20 17:36 UTC (permalink / raw)
  To: D Scott Phillips
  Cc: linux-arm-kernel, Catalin Marinas, Will Deacon, Darren Hart, patches

On Mon, 20 Dec 2021 16:40:45 +0000,
D Scott Phillips <scott@os.amperecomputing.com> wrote:
> 
> Marc Zyngier <maz@kernel.org> writes:
> 
> > On Fri, 17 Dec 2021 21:19:20 +0000,
> > D Scott Phillips <scott@os.amperecomputing.com> wrote:
> >> 
> >> The erratum 1418040 workaround enables vct access trapping when executing
> >
> > nit: s/vct/CNTVCT_EL1/.
> 
> fixed, thanks
> 
> >> 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 <scott@os.amperecomputing.com>
> >> Cc: <stable@vger.kernel.org> # 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.
> 
> Yes, makes sense. I've added an erratum_1418040_new_exec() helper that
> does preempt_disable/erratum_14518040_thread_switch(current)/preempt_enable,
> and called it from arch_setup_new_exec().

Yes, that's much better.

> 
> >>
> >> -	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?
> 
> The workaround in __switch_to isn't happening for every return to
> userspace, but rather for every scheduler task switch. When a process
> exec()s, no switch happens at that point. From the scheduler point of
> view, this is still the same task.  So in the time period between
> exec()ing a compat task from non-compat (or vice versa) and the first
> time it gets switched out, we would apply the wrong workaround state,
> unless we make a change to cntkctl from exec() before returning back to
> EL0.

Right. For $reason, I keep equating task switch and return to user.
Thanks for spelling it out for me.

	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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-12-20 17:37 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-17 21:19 [PATCH v4] arm64: errata: Fix exec handling in erratum 1418040 workaround D Scott Phillips
2021-12-17 21:36 ` D Scott Phillips
2021-12-20 12:03 ` Marc Zyngier
2021-12-20 16:40   ` D Scott Phillips
2021-12-20 17:36     ` Marc Zyngier

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.