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

From: Borislav Petkov
Date: Wed Jun 23 2021 - 04:10:55 EST


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