linux-kernel - Re: [PATCH 1/1] headers/deps: x86/fpu: Make task

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]

Date: Tue, 26 Mar 2024 20:12:31 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Oleg Nesterov <oleg@...hat.com>
Cc: linux-kernel@...r.kernel.org, Andy Lutomirski <luto@...capital.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Dave Hansen <dave@...1.net>, Peter Zijlstra <peterz@...radead.org>,
	Borislav Petkov <bp@...en8.de>, "H . Peter Anvin" <hpa@...or.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Uros Bizjak <ubizjak@...il.com>
Subject: Re: [PATCH 1/1] headers/deps: x86/fpu: Make task_struct::thread
 constant size


* Oleg Nesterov <oleg@...hat.com> wrote:

> On 03/20, Ingo Molnar wrote:
> >
> > --- a/arch/x86/kernel/fpu/init.c
> > +++ b/arch/x86/kernel/fpu/init.c
> > @@ -38,7 +38,7 @@ static void fpu__init_cpu_generic(void)
> >  	/* Flush out any pending x87 state: */
> >  #ifdef CONFIG_MATH_EMULATION
> >  	if (!boot_cpu_has(X86_FEATURE_FPU))
> > -		fpstate_init_soft(&current->thread.fpu.fpstate->regs.soft);
> > +		fpstate_init_soft(current->thread.fpu->fpstate->regs.soft);
>                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Typo? it should be
> 
> 	&current->thread.fpu->fpstate->regs.soft

Thx, fixed.

> 
> > +static struct fpu x86_init_fpu __read_mostly;
> > +
> >  static void __init fpu__init_system_early_generic(void)
> >  {
> > +	{
> > +		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;
> > +	}
> 
> Can't x86_init_fpu be declared inside the block above?

As a function-local static? I think globals are better defined in a visible 
fashion, not hidden among local variables where they are easy to overlook.

But the extra block is unnecessary in any case.

> >  void __init fpu__init_system(void)
> >  {
> > -	fpstate_reset(&current->thread.fpu);
> >  	fpu__init_system_early_generic();
> > +	fpstate_reset(current->thread.fpu);
> 
> It seems that fpstate_reset(current->thread.fpu) is not needed after the
> change in fpu__init_system_early_generic() above.

Yeah. Something like the delta patch below?

Thanks,

	Ingo

=================>

 arch/x86/kernel/fpu/init.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index f9412d012191..de618ec509aa 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -38,7 +38,7 @@ static void fpu__init_cpu_generic(void)
 	/* Flush out any pending x87 state: */
 #ifdef CONFIG_MATH_EMULATION
 	if (!boot_cpu_has(X86_FEATURE_FPU))
-		fpstate_init_soft(current->thread.fpu->fpstate->regs.soft);
+		fpstate_init_soft(&current->thread.fpu->fpstate->regs.soft);
 	else
 #endif
 		asm volatile ("fninit");
@@ -75,14 +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;
-	}
+	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;
 
 	if (!boot_cpu_has(X86_FEATURE_CPUID) &&
 	    !test_bit(X86_FEATURE_FPU, (unsigned long *)cpu_caps_cleared)) {
@@ -222,7 +220,6 @@ static void __init fpu__init_system_xstate_size_legacy(void)
 void __init fpu__init_system(void)
 {
 	fpu__init_system_early_generic();
-	fpstate_reset(current->thread.fpu);
 
 	/*
 	 * The FPU has to be operational for some of the