linux-kernel - Re: [PATCH 10/11] x86/fpu: prepare copy_fpstate_to_sigframe for TIF_LOAD

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 12 Oct 2018 12:40:19 -0700
From:   Dave Hansen <dave.hansen@...ux.intel.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        linux-kernel@...r.kernel.org
Cc:     x86@...nel.org, Andy Lutomirski <luto@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        kvm@...r.kernel.org, "Jason A. Donenfeld" <Jason@...c4.com>,
        Rik van Riel <riel@...riel.com>
Subject: Re: [PATCH 10/11] x86/fpu: prepare copy_fpstate_to_sigframe for
 TIF_LOAD_FPU

On 10/04/2018 07:05 AM, Sebastian Andrzej Siewior wrote:
> From: Rik van Riel <riel@...riel.com>
> 
> If TIF_LOAD_FPU is set, then the registers are saved (not loaded). In that case
> we skip the saving part.

This sentence hurts my brain.

	"If TIF_LOAD_FPU is set the registers are ... not loaded"

I think that means that something could use some better naming.

Should TIF_LOAD_FPU be TIF_NEED_FPU_LOAD, perhaps?

> Signed-off-by: Rik van Riel <riel@...riel.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> ---
>  arch/x86/kernel/fpu/signal.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index c8f5ff58578ed..979dcd1ed82e0 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -155,13 +155,17 @@ 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;
>  
> -	/* Update the thread's fxstate to save the fsave header. */
> -	if (ia32_fxstate) {
> -		copy_fxregs_to_kernel(fpu);
> -	} else {
> -		copy_fpregs_to_fpstate(fpu);
> -		fpregs_deactivate(fpu);
> +	__fpregs_changes_begin();
> +	if (!test_thread_flag(TIF_LOAD_FPU)) {

This needs commenting, please.

If we do not need to load the FPU at return to userspace, it means the
state is in the the registers, not the buffer.  So, update the buffer to
match the registers.

> +		/* Update the thread's fxstate to save the fsave header. */
> +		if (ia32_fxstate) {
> +			copy_fxregs_to_kernel(fpu);
> +		} else {
> +			copy_fpregs_to_fpstate(fpu);
> +			fpregs_deactivate(fpu);
> +		}
>  	}
> +	__fpregs_changes_end();

Do we really need the __fpregs_changes_*() abstraction for this single
call site?