Re: [PATCH v2] arm64: errata: Fix exec handling in erratum 1418040 workaround

From: Catalin Marinas <catalin.marinas@arm.com>
To: D Scott Phillips <scott@os.amperecomputing.com>
Cc: linux-arm-kernel@lists.infradead.org,
	Marc Zyngier <maz@kernel.org>, Will Deacon <will@kernel.org>,
	Darren Hart <darren@os.amperecomputing.com>,
	patches@amperecomputing.com
Subject: Re: [PATCH v2] arm64: errata: Fix exec handling in erratum 1418040 workaround
Date: Fri, 17 Dec 2021 18:38:57 +0000	[thread overview]
Message-ID: <YbzZQexVR6eULEVa@arm.com> (raw)
In-Reply-To: <86r1abtc86.fsf@scott-ph-mail.amperecomputing.com>

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