Re: [patch V3 63/66] x86/fpu/signal: Split out the direct restore code

[Borislav Petkov]

On Fri, Jun 18, 2021 at 04:19:26PM +0200, Thomas Gleixner wrote:
> Prepare for smarter failure handling of the direct restore.
> 
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/fpu/signal.c |  110 +++++++++++++++++++++----------------------
>  1 file changed, 56 insertions(+), 54 deletions(-)
> 
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -249,10 +249,7 @@ sanitize_restored_user_xstate(union fpre
>  	}
>  }
>  
> -/*
> - * Restore the FPU state directly from the userspace signal frame.
> - */
> -static int restore_fpregs_from_user(void __user *buf, u64 xrestore, bool fx_only)
> +static int restore_hwregs_from_user(void __user *buf, u64 xrestore, bool fx_only)

Or simply

	__restore_fpregs_from_user

to denote it is the low-level helper like the rest of the code does
around here.

>  {
>  	if (use_xsave()) {
>  		u64 init_bv = xfeatures_mask_uabi() & ~xrestore;
> @@ -273,6 +270,56 @@ static int restore_fpregs_from_user(void
>  	}
>  }
>  
> +static int restore_fpregs_from_user(void __user *buf, u64 xrestore, bool fx_only)
> +{
> +	struct fpu *fpu = &current->thread.fpu;
> +	int ret ;
	       ^

superfluous space.


> +	fpregs_lock();
> +	pagefault_disable();
> +	ret = restore_hwregs_from_user(buf, xrestore, fx_only);
> +	pagefault_enable();
> +
> +	if (unlikely(ret)) {
> +		/*
> +		 * The above did an FPU restore operation, restricted to
> +		 * the user portion of the registers, and failed, but the
> +		 * microcode might have modified the FPU registers
> +		 * nevertheless.
> +		 *
> +		 * If the FPU registers do not belong to current, then
> +		 * invalidate the FPU register state otherwise the task
> +		 * might preempt current and return to user space with
> +		 * corrupted FPU registers.
> +		 *
> +		 * In case current owns the FPU registers then no further
> +		 * action is required. The fixup in the slow path will
> +		 * handle it correctly.
> +		 */
> +		if (test_thread_flag(TIF_NEED_FPU_LOAD))
> +			__cpu_invalidate_fpregs_state();
> +		fpregs_unlock();
> +		return ret;
> +	}
> +
> +	/*
> +	 * Restore supervisor states: previous context switch etc has done
> +	 * XSAVES and saved the supervisor states in the kernel buffer from
> +	 * which they can be restored now.
> +	 *
> +	 * We cannot do a single XRSTORS here - which would be nice -

Might wanna fix up the "We" brotherhood formulation, while at it. :)

> +	 * because the rest of the FPU registers are being restored from a
> +	 * user buffer directly. The single XRSTORS happens below, when the
> +	 * user buffer has been copied to the kernel one.
> +	 */
> +	if (test_thread_flag(TIF_NEED_FPU_LOAD) && xfeatures_mask_supervisor())
> +		os_xrstor(&fpu->state.xsave, xfeatures_mask_supervisor());
> +
> +	fpregs_mark_activate();
> +	fpregs_unlock();
> +	return 0;

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg