[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zmq9ppuIZJ9IMZDr@gmail.com>
Date: Thu, 13 Jun 2024 11:36:38 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Nathan Chancellor <nathan@...nel.org>, 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>, Brian Gerst <brgerst@...il.com>,
"H . Peter Anvin" <hpa@...or.com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Thomas Gleixner <tglx@...utronix.de>,
Uros Bizjak <ubizjak@...il.com>
Subject: [PATCH 10/9] x86/fpu: Fix 'struct fpu' misalignment on 32-bit kernels
* Oleg Nesterov <oleg@...hat.com> wrote:
> The patch below seems to fix the problem.
>
> Again, the changes in fpu__init_system_early_generic() are not
> strictly needed to fix it, but I believe make sense anyway.
>
> Oleg.
>
>
> diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
> index 4e8d37b5a90b..848ea79886ba 100644
> --- a/arch/x86/kernel/fpu/init.c
> +++ b/arch/x86/kernel/fpu/init.c
> @@ -71,16 +71,14 @@ static bool __init fpu__probe_without_cpuid(void)
> return fsw == 0 && (fcw & 0x103f) == 0x003f;
> }
>
> -static struct fpu x86_init_fpu __read_mostly;
> +static struct fpu x86_init_fpu __attribute__ ((aligned (64))) __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;
> + 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)) {
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 215a7380e41c..ec22b9bf27f5 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1562,7 +1562,7 @@ struct task_struct {
> * they are included in the randomized portion of task_struct.
> */
> randomized_struct_fields_end
> -};
> +} __attribute__ ((aligned (64)));
Oh ... indeed, FPU context save area must be 64 bytes aligned!
On 64-bit kernels this was a given, accidentally, but on 32-bit kernels
init_task was only 32-byte aligned:
c22f04e0 D init_task
... which misaligned the struct fpu as well, I think. With your fix:
c22f0500 D init_task
What happened is that due to my series 'struct task_struct' lost its
64-byte alignment attribute, which broke the fpu struct allocation code on
32-bit kernels and made the 64-bit one probably unrobust as well.
To add insult to injury, I was aware of the alignment requirement, and
tried to cover it with an assert, but doubly mis-coded it:
+ BUILD_BUG_ON(sizeof(*dst) % SMP_CACHE_BYTES != 0);
Which is buggy:
- As on 32-bit kernels CONFIG_X86_L1_CACHE_SHIFT=5, ie. 32 bytes ...
- Nor does it really check the alignment of the FPU context save area
within struct fpu as it's allocated after task_struct ...
The interim patch below against the full WIP.x86/fpu series is what fixes
Nathan's 32-bit testcase.
Further improvements:
- The extra alignment attribute in <linux/sched.h> will affect other
architecture as well, although in practice the alignment of init_task is
not critical, and is very likely at least 32 bytes, probably more.
Still, it's a bit ugly in its current form.
- Also, because this was pretty hard to debug, we should probably add an
alignment check to fpu__init_task_struct_size() where we allocate the
fpu context structure, and fix the buggy size-assert.
Thanks a lot for your help Oleg! I've added this tag of yours:
Fixed-by: Oleg Nesterov <oleg@...hat.com>
... and would appreciate your Acked-by or Reviewed-by for the eventual
final version of the series, but I don't insist. ;-)
Ingo
=================>
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 53580e59e5db..16b6611634c3 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -71,15 +71,13 @@ static bool __init fpu__probe_without_cpuid(void)
return fsw == 0 && (fcw & 0x103f) == 0x003f;
}
-static struct fpu x86_init_fpu __read_mostly;
+static struct fpu x86_init_fpu __attribute__ ((aligned (64))) __read_mostly;
static void __init fpu__init_system_early_generic(void)
{
- int this_cpu = smp_processor_id();
-
fpstate_reset(&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)) {
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 215a7380e41c..ec22b9bf27f5 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1562,7 +1562,7 @@ struct task_struct {
* they are included in the randomized portion of task_struct.
*/
randomized_struct_fields_end
-};
+} __attribute__ ((aligned (64)));
#define TASK_REPORT_IDLE (TASK_REPORT + 1)
#define TASK_REPORT_MAX (TASK_REPORT_IDLE << 1)
Powered by blists - more mailing lists