Re: [PATCH v10 13/28] x86/fpu/xstate: Use feature disable (XFD) to protect dynamic user state

[Thomas Gleixner <tglx@linutronix.de>]

Chang,

On Fri, Oct 01 2021 at 17:02, Thomas Gleixner wrote:
> On Wed, Aug 25 2021 at 08:53, Chang S. Bae wrote:
>> +/**
>> + * xfd_switch - Switches the MSR IA32_XFD context if needed.
>> + * @prev:	The previous task's struct fpu pointer
>> + * @next:	The next task's struct fpu pointer
>> + */
>> +static inline void xfd_switch(struct fpu *prev, struct fpu *next)
>> +{
>> +	u64 prev_xfd_mask, next_xfd_mask;
>> +
>> +	if (!cpu_feature_enabled(X86_FEATURE_XFD) || !xfeatures_mask_user_dynamic)
>> +		return;
>
> This is context switch, so this wants to be a static key which is turned
> on during init when the CPU supports XFD and user dynamic features are
> available.
>
>> +
>> +	prev_xfd_mask = prev->state_mask & xfeatures_mask_user_dynamic;
>> +	next_xfd_mask = next->state_mask & xfeatures_mask_user_dynamic;
>> +
>> +	if (unlikely(prev_xfd_mask != next_xfd_mask))
>> +		wrmsrl_safe(MSR_IA32_XFD, xfeatures_mask_user_dynamic ^ next_xfd_mask);
>> +}
>> +
>>  /*
>>   * Delay loading of the complete FPU state until the return to userland.
>>   * PKRU is handled separately.
>>   */
>> -static inline void switch_fpu_finish(struct fpu *new_fpu)
>> +static inline void switch_fpu_finish(struct fpu *old_fpu, struct fpu *new_fpu)
>>  {
>> -	if (cpu_feature_enabled(X86_FEATURE_FPU))
>> +	if (cpu_feature_enabled(X86_FEATURE_FPU)) {
>>  		set_thread_flag(TIF_NEED_FPU_LOAD);
>> +		xfd_switch(old_fpu, new_fpu);
>
> Why has this to be done on context switch? Zero explanation provided.
>
> Why can't this be done in exit_to_user() where the FPU state restore is
> handled?

DEFINE_PER_CPU(xfd_state);

update_xfd(fpu)
{
	if (__this_cpu_read(xfd_state) != fpu->xfd_state) {
		wrmsrl(XFD, fpu->xfd_state);
		__this_cpu_write(xfd_state, fpu->xfd_state);
	}
}

fpregs_restore_userregs()
{
	if (!fpregs_state_valid(fpu, cpu)) {
        	if (static_branch_unlikely(xfd_switching_enabled))
                	update_xfd(fpu);
	...
	}
}

Hmm?

Thanks,

        tglx