linux-kernel - Re: [PATCH] x86: Use __fpu_invalidate_fpregs

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <2398119e-a03e-4bf9-12e7-722fb23f8c72@intel.com>
Date:   Tue, 22 Aug 2023 13:10:57 -0700
From:   Sohil Mehta <sohil.mehta@...el.com>
To:     Rick Edgecombe <rick.p.edgecombe@...el.com>, <tglx@...utronix.de>,
        <mingo@...hat.com>, <bp@...en8.de>, <x86@...nel.org>,
        <hpa@...or.com>, <dave.hansen@...ux.intel.com>, <luto@...nel.org>,
        <peterz@...radead.org>
CC:     <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86: Use __fpu_invalidate_fpregs_state() in exec

Hi Rick,

The comment below doesn't change anything meaningfully, but mainly helps
clarifies the original comment.

Maybe something like this:
>   * If the FPU register state is valid, the kernel can skip restoring the
>   * FPU state from memory.
>   *
> - * Any code that clobbers the FPU registers or updates the in-memory
> - * FPU state for a task MUST let the rest of the kernel know that the
> - * FPU registers are no longer valid for this task.
> + * Any code that clobbers the FPU registers or updates the in-memory FPU state
> + * for a task MUST let the rest of the kernel know that whether the FPU
> + * registers are no longer valid anywhere for this task or if the FPU state on
> + * a particular CPU is invalid.
>   *
> - * Either one of these invalidation functions is enough. Invalidate
> - * a resource you control: CPU if using the CPU for something else
> - * (with preemption disabled), FPU for the current task, or a task that
> - * is prevented from running by the current task.
> + * Invalidate a resource that you truly control:
> + * - Use __cpu_invalidate_fpregs_state() or equivalent when using the CPU for
> + *   something else (only with preemption disabled)
> + * - Use __fpu_invalidate_fpregs_state() or equivalent when modifying the FPU
> + *   for the current task, or a task that is prevented from running by the
> + *   current task.
>   */

Please disregard my suggestion if you think the existing one is
sufficient for now.

On 8/18/2023 10:03 AM, Rick Edgecombe wrote:
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index e03b6b107b20..a86d37052a64 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -713,7 +713,7 @@ static void fpu_reset_fpregs(void)
>  	struct fpu *fpu = &current->thread.fpu;
>  
>  	fpregs_lock();
> -	fpu__drop(fpu);
> +	__fpu_invalidate_fpregs_state(fpu);
>  	/*
>  	 * This does not change the actual hardware registers. It just
>  	 * resets the memory image and sets TIF_NEED_FPU_LOAD so a

The code fix looks fine to me.

Even if you don't update the comment, please feel free to add:
Reviewed-by: Sohil Mehta <sohil.mehta@...el.com>