linux-kernel - Re: [PATCH v1 3/3] x86/entry: Set FRED RSP0 on return to userspace instead of context switch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eb2ed1b4-94cf-4d3d-b726-6ee0fa13ca9e@suse.com>
Date: Fri, 9 Aug 2024 13:45:43 +0300
From: Nikolay Borisov <nik.borisov@...e.com>
To: "Xin Li (Intel)" <xin@...or.com>, linux-kernel@...r.kernel.org
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
 peterz@...radead.org, andrew.cooper3@...rix.com, seanjc@...gle.com
Subject: Re: [PATCH v1 3/3] x86/entry: Set FRED RSP0 on return to userspace
 instead of context switch



On 7.08.24 г. 8:47 ч., Xin Li (Intel) wrote:
> FRED RSP0 is a per task constant pointing to top of its kernel stack
> for user level event delivery, and needs to be updated when a task is
> scheduled in.
> 
> Introduce a new TI flag TIF_LOAD_USER_STATES to track whether FRED RSP0
> needs to be loaded, and do the actual load of FRED RSP0 in
> arch_exit_to_user_mode_prepare() if the TI flag is set, thus to avoid a
> fair number of WRMSRs in both KVM and the kernel.
> 
> Suggested-by: Sean Christopherson <seanjc@...gle.com>
> Signed-off-by: Xin Li (Intel) <xin@...or.com>
> ---
>   arch/x86/include/asm/entry-common.h | 5 +++++
>   arch/x86/include/asm/switch_to.h    | 3 +--
>   arch/x86/include/asm/thread_info.h  | 2 ++
>   arch/x86/kernel/cpu/cpuid-deps.c    | 1 -
>   4 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/entry-common.h b/arch/x86/include/asm/entry-common.h
> index 4c78b99060b5..ae365579efb3 100644
> --- a/arch/x86/include/asm/entry-common.h
> +++ b/arch/x86/include/asm/entry-common.h
> @@ -51,6 +51,11 @@ static inline void arch_exit_to_user_mode_prepare(struct pt_regs *regs,
>   		if (ti_work & _TIF_USER_RETURN_NOTIFY)
>   			fire_user_return_notifiers();
>   
> +		if (cpu_feature_enabled(X86_FEATURE_FRED) &&
> +		    (ti_work & _TIF_LOAD_USER_STATES))
> +			wrmsrns(MSR_IA32_FRED_RSP0,
> +				(unsigned long)task_stack_page(current) + THREAD_SIZE);
> +
>   		if (unlikely(ti_work & _TIF_IO_BITMAP))
>   			tss_update_io_bitmap();
>   
> diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
> index c3bd0c0758c9..a31ea544cc0e 100644
> --- a/arch/x86/include/asm/switch_to.h
> +++ b/arch/x86/include/asm/switch_to.h
> @@ -71,8 +71,7 @@ static inline void update_task_stack(struct task_struct *task)
>   	this_cpu_write(cpu_tss_rw.x86_tss.sp1, task->thread.sp0);
>   #else
>   	if (cpu_feature_enabled(X86_FEATURE_FRED)) {
> -		/* WRMSRNS is a baseline feature for FRED. */
> -		wrmsrns(MSR_IA32_FRED_RSP0, (unsigned long)task_stack_page(task) + THREAD_SIZE);
> +		set_thread_flag(TIF_LOAD_USER_STATES);
>   	} else if (cpu_feature_enabled(X86_FEATURE_XENPV)) {
>   		/* Xen PV enters the kernel on the thread stack. */
>   		load_sp0(task_top_of_stack(task));
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index 12da7dfd5ef1..fb51904651c0 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -106,6 +106,7 @@ struct thread_info {
>   #define TIF_BLOCKSTEP		25	/* set when we want DEBUGCTLMSR_BTF */
>   #define TIF_LAZY_MMU_UPDATES	27	/* task is updating the mmu lazily */
>   #define TIF_ADDR32		29	/* 32-bit address space on 64 bits */
> +#define TIF_LOAD_USER_STATES	30	/* Load user level states */

Wouldn't something along the l ines of TIF_LOAD_FRED_RSP be more 
descriptive, or it's expected that this flag can cover more state in the 
future?

>   
>   #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
>   #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
> @@ -128,6 +129,7 @@ struct thread_info {
>   #define _TIF_BLOCKSTEP		(1 << TIF_BLOCKSTEP)
>   #define _TIF_LAZY_MMU_UPDATES	(1 << TIF_LAZY_MMU_UPDATES)
>   #define _TIF_ADDR32		(1 << TIF_ADDR32)
> +#define _TIF_LOAD_USER_STATES	(1 << TIF_LOAD_USER_STATES)
>   
>   /* flags to check in __switch_to() */
>   #define _TIF_WORK_CTXSW_BASE					\
> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
> index b7d9f530ae16..8bd84114c2d9 100644
> --- a/arch/x86/kernel/cpu/cpuid-deps.c
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -83,7 +83,6 @@ static const struct cpuid_dep cpuid_deps[] = {
>   	{ X86_FEATURE_AMX_TILE,			X86_FEATURE_XFD       },
>   	{ X86_FEATURE_SHSTK,			X86_FEATURE_XSAVES    },
>   	{ X86_FEATURE_FRED,			X86_FEATURE_LKGS      },
> -	{ X86_FEATURE_FRED,			X86_FEATURE_WRMSRNS   },
>   	{}
>   };
>