linux-kernel - question about switch_fpu_prepare/switch_fpu

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: <20250422170059.GB1676@redhat.com>
Date: Tue, 22 Apr 2025 19:01:00 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Ingo Molnar <mingo@...nel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Cc: linux-kernel@...r.kernel.org, Andy Lutomirski <luto@...capital.net>,
	Dave Hansen <dave@...1.net>, Brian Gerst <brgerst@...il.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Borislav Petkov <bp@...en8.de>, "H . Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"Chang S . Bae" <chang.seok.bae@...el.com>
Subject: question about switch_fpu_prepare/switch_fpu_finish

I must have missed something, but I can't understand this logic, it
seems unnecessarily complicated today.

1. Now that switch_fpu_finish() doesn't load the FPU state, I think it
   can be folded into switch_fpu_prepare().

2. But the main question is that I fail to understand why
   __switch_to() -> switch_fpu_finish() uses the "next" task to set
   TIF_NEED_FPU_LOAD.

   I think that set_tsk_thread_flag(prev_p, TIF_NEED_FPU_LOAD) makes
   more sense.

   Just in case, note that fpu_clone() sets TIF_NEED_FPU_LOAD, so
   we should not worry about the 1st __switch_to(next_p).

IOW, can you explain why the (untested) patch below could be wrong?

We can even remove the PF_KTHREAD check in switch_fpu_prepare(), kthreads
should never clear TIF_NEED_FPU_LOAD...

Oleg.
---

diff --git a/arch/x86/include/asm/fpu/sched.h b/arch/x86/include/asm/fpu/sched.h
index 5fd12634bcc4..cdd60f434289 100644
--- a/arch/x86/include/asm/fpu/sched.h
+++ b/arch/x86/include/asm/fpu/sched.h
@@ -54,18 +54,10 @@ static inline void switch_fpu_prepare(struct task_struct *old, int cpu)
 		 */
 		old_fpu->last_cpu = cpu;
 
+		set_tsk_thread_flag(old, TIF_NEED_FPU_LOAD);
+
 		trace_x86_fpu_regs_deactivated(old_fpu);
 	}
 }
 
-/*
- * Delay loading of the complete FPU state until the return to userland.
- * PKRU is handled separately.
- */
-static inline void switch_fpu_finish(struct task_struct *new)
-{
-	if (cpu_feature_enabled(X86_FEATURE_FPU))
-		set_tsk_thread_flag(new, TIF_NEED_FPU_LOAD);
-}
-
 #endif /* _ASM_X86_FPU_SCHED_H */
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 4636ef359973..b398a6ef2923 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -208,8 +208,6 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 
 	raw_cpu_write(current_task, next_p);
 
-	switch_fpu_finish(next_p);
-
 	/* Load the Intel cache allocation PQR MSR. */
 	resctrl_sched_in(next_p);
 
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 7196ca7048be..e8262e637ea4 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -671,8 +671,6 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	raw_cpu_write(current_task, next_p);
 	raw_cpu_write(cpu_current_top_of_stack, task_top_of_stack(next_p));
 
-	switch_fpu_finish(next_p);
-
 	/* Reload sp0. */
 	update_task_stack(next_p);