Received-SPF: pass (google.com: domain of linux-kernel+bounces-211283-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 12 Jun 2024 11:40:39 +0200
From: Oleg Nesterov <oleg@redhat.com>
To: Ingo Molnar <mingo@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>, linux-kernel@vger.kernel.org,
	Andy Lutomirski <luto@amacapital.net>,
	Andrew Morton <akpm@linux-foundation.org>,
	Dave Hansen <dave@sr71.net>, Peter Zijlstra <peterz@infradead.org>,
	Borislav Petkov <bp@alien8.de>, Brian Gerst <brgerst@gmail.com>,
	"H . Peter Anvin" <hpa@zytor.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Uros Bizjak <ubizjak@gmail.com>
Subject: Re: [PATCH 3/9] x86/fpu: Make task_struct::thread constant size
Message-ID: <20240612094039.GA23973@redhat.com>
References: <20240608073134.264210-1-mingo@kernel.org>
 <20240608073134.264210-4-mingo@kernel.org>
 <20240610211350.GA1613053@thelio-3990X>
 <20240611124145.GA26798@redhat.com>
 <ZmlZiHVF8w09mExw@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZmlZiHVF8w09mExw@gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)

On 06/12, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > > +	per_cpu(fpu_fpregs_owner_ctx, this_cpu) = &x86_init_fpu;
> > > +	x86_init_fpu.last_cpu = this_cpu;
> >
> > Why? I think it should do
> >
> > 	x86_init_fpu.last_cpu = -1;
> > 	set_thread_flag(TIF_NEED_FPU_LOAD);
> >
> > And the next patch should kill x86_init_fpu altogether, but keep
> > TIF_NEED_FPU_LOAD. It should be never cleared if PF_KTHREAD.
>
> So I applied the patch further below on top of:
>
>    4f4a9b399357 x86/fpu: Make task_struct::thread constant size
>
> And Nathan's 32-bit kernel testcase [but running with 1 CPU to simplify it]
> still crashes in a similar fashion

Yes, I didn't expect it can fix the problem. Still makes sense, I think.

> in the (first?) modprobe instance with a
> bad FPU state exception:

OK, I reproduced it too. I see nothing wrong in the usermodehelper or
kernel_execve paths... and fpu_clone() looks fine, "minimal" is still
true if init_task or another PF_KTHREAD calls user_mode_thread().

So I appiled the patch below and save_fpregs_to_fpstate() in
fpu__init_system() triggers the WARN_ON_FPU(err) in os_xsave()

	[    0.014609] RESTORED !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
	[    0.014958] ------------[ cut here ]------------
	[    0.014958] WARNING: CPU: 0 PID: 0 at arch/x86/kernel/fpu/xstate.h:189 save_fpregs_to_fpstate+0x74/0x80
	...

so I _think_ we can probably forget about modprobe/etc.

Oleg.


diff --git a/arch/x86/kernel/fpu/context.h b/arch/x86/kernel/fpu/context.h
index 10d0a720659c..9fa78f75b2e5 100644
--- a/arch/x86/kernel/fpu/context.h
+++ b/arch/x86/kernel/fpu/context.h
@@ -56,8 +56,8 @@ static inline void fpregs_restore_userregs(void)
 	struct fpu *fpu = x86_task_fpu(current);
 	int cpu = smp_processor_id();
 
-	if (WARN_ON_ONCE(current->flags & (PF_KTHREAD | PF_USER_WORKER)))
-		return;
+//	if (WARN_ON_ONCE(current->flags & (PF_KTHREAD | PF_USER_WORKER)))
+//		return;
 
 	if (!fpregs_state_valid(fpu, cpu)) {
 		/*
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 4e8d37b5a90b..0e63d54595aa 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -5,11 +5,11 @@
 #include <asm/fpu/api.h>
 #include <asm/tlbflush.h>
 #include <asm/setup.h>
-
+#include <asm/fpu/signal.h>
 #include <linux/sched.h>
 #include <linux/sched/task.h>
 #include <linux/init.h>
-
+#include "context.h"
 #include "internal.h"
 #include "legacy.h"
 #include "xstate.h"
@@ -75,12 +75,12 @@ static struct fpu x86_init_fpu __read_mostly;
 
 static void __init fpu__init_system_early_generic(void)
 {
-	int this_cpu = smp_processor_id();
+//	int this_cpu = smp_processor_id();
 
 	fpstate_reset(&x86_init_fpu);
 	current->thread.fpu = &x86_init_fpu;
-	per_cpu(fpu_fpregs_owner_ctx, this_cpu) = &x86_init_fpu;
-	x86_init_fpu.last_cpu = this_cpu;
+	set_thread_flag(TIF_NEED_FPU_LOAD);
+	x86_init_fpu.last_cpu = -1;
 
 	if (!boot_cpu_has(X86_FEATURE_CPUID) &&
 	    !test_bit(X86_FEATURE_FPU, (unsigned long *)cpu_caps_cleared)) {
@@ -217,6 +217,7 @@ static void __init fpu__init_system_xstate_size_legacy(void)
  * Called on the boot CPU once per system bootup, to set up the initial
  * FPU state that is later cloned into all processes:
  */
+void save_fpregs_to_fpstate(struct fpu *fpu);
 void __init fpu__init_system(void)
 {
 	fpu__init_system_early_generic();
@@ -231,4 +232,10 @@ void __init fpu__init_system(void)
 	fpu__init_system_xstate_size_legacy();
 	fpu__init_system_xstate(fpu_kernel_cfg.max_size);
 	fpu__init_task_struct_size();
+
+	BUG_ON(x86_task_fpu(current) != &x86_init_fpu);
+	fpregs_restore_userregs();
+	pr_crit("RESTORED !!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n");
+	save_fpregs_to_fpstate(&x86_init_fpu);
+	pr_crit("SAVED !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n");
 }