Re: [PATCH 18/24] x86/fpu: Prepare copy_fpstate_to_sigframe() for TIF_NEED_FPU_LOAD

[Thomas Gleixner]

On Thu, 21 Mar 2019, Sebastian Andrzej Siewior wrote:

> From: Rik van Riel <riel@xxxxxxxxxxx>
> 
> The FPU registers need only to be saved if TIF_NEED_FPU_LOAD is not set.
> Otherwise this has been already done and can be skipped.
> 
> Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/fpu/signal.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index f55f16d9e7e4e..97ea6909ede1f 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -155,7 +155,16 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
>  			sizeof(struct user_i387_ia32_struct), NULL,
>  			(struct _fpstate_32 __user *) buf) ? -1 : 1;
>  
> -	copy_fpregs_to_fpstate(fpu);
> +	fpregs_lock();
> +	/*
> +	 * If we do not need to load the FPU registers at return to userspace
> +	 * then the CPU has the current state and we need to save it. Otherwise
> +	 * it is already done and we can skip it.
> +	 */
> +	if (!test_thread_flag(TIF_NEED_FPU_LOAD))
> +		copy_fpregs_to_fpstate(fpu);

I think this should do the following:

	fpregs_lock();
	if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
		pagefault_disable();
		ret = copy_fpu_to_user(...);
		pagefault_enable();
		if (!res)
			return 0;
		copy_fpregs_to_fpstate(fpu); 
	}
	fpregs_unlock();

The point is that in most cases the direct store from the FPU registers to
user space will succeed simply because the stack is accessible and you only
do the store in kernel memory and copy when that fails.

Thanks,

	tglx