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

From: D Scott Phillips <scott@os.amperecomputing.com>
To: Catalin Marinas <catalin.marinas@arm.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 08:42:01 -0800	[thread overview]
Message-ID: <86r1abtc86.fsf@scott-ph-mail.amperecomputing.com> (raw)
In-Reply-To: <YbyhqB7XP9tHMfmE@arm.com>

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