On 32bit FS and on 64bit GS segments are already set up correctly, but
load_percpu_segment() still sets [FG]S after switching from the early GDT
to the direct GDT.
For 32bit the segment load has no side effects, but on 64bit it causes
GSBASE to become 0, which means that any per CPU access before GSBASE is
set to the new value is going to fault. That's the reason why the whole
file containing this code has stackprotector removed.
But that's a pointless exercise for both 32 and 64 bit as the relevant
segment selector is already correct. Loading the new GDT does not change
that.
Remove the segment loads and inline load_percpu_segment() into the only
caller. Add comments while at it.
Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/include/asm/processor.h | 1
arch/x86/kernel/cpu/common.c | 42 ++++++++++++++++++++++++---------------
2 files changed, 26 insertions(+), 17 deletions(-)
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -673,7 +673,6 @@ extern struct desc_ptr early_gdt_descr;
extern void switch_to_new_gdt(int);
extern void load_direct_gdt(int);
extern void load_fixmap_gdt(int);
-extern void load_percpu_segment(int);
extern void cpu_init(void);
extern void cpu_init_secondary(void);
extern void cpu_init_exception_handling(void);
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -701,16 +701,6 @@ static const char *table_lookup_model(st
__u32 cpu_caps_cleared[NCAPINTS + NBUGINTS] __aligned(sizeof(unsigned long));
__u32 cpu_caps_set[NCAPINTS + NBUGINTS] __aligned(sizeof(unsigned long));
-void load_percpu_segment(int cpu)
-{
-#ifdef CONFIG_X86_32
- loadsegment(fs, __KERNEL_PERCPU);
-#else
- __loadsegment_simple(gs, 0);
- wrmsrl(MSR_GS_BASE, cpu_kernelmode_gs_base(cpu));
-#endif
-}
-
#ifdef CONFIG_X86_32
/* The 32-bit entry code needs to find cpu_entry_area. */
DEFINE_PER_CPU(struct cpu_entry_area *, cpu_entry_area);
@@ -738,16 +728,36 @@ void load_fixmap_gdt(int cpu)
}
EXPORT_SYMBOL_GPL(load_fixmap_gdt);
-/*
- * Current gdt points %fs at the "master" per-cpu area: after this,
- * it's on the real one.
+/**
+ * switch_to_new_gdt - Switch form early GDT to the direct one
+ * @cpu: The CPU number for which this is invoked
+ *
+ * Invoked during early boot to switch from early GDT and early per CPU
+ * (%fs on 32bit, GS_BASE on 64bit) to the direct GDT and the runtime per
+ * CPU area.
*/
void switch_to_new_gdt(int cpu)
{
- /* Load the original GDT */
load_direct_gdt(cpu);
- /* Reload the per-cpu base */
- load_percpu_segment(cpu);
+
+ /*
+ * No need to load the %gs (%fs for 32bit) segment. It is already
+ * correct, %gs is 0 on 64bit and %fs is __KERNEL_PERCPU on 32 bit.
+ *
+ * Writing %gs on 64bit would zero GSBASE which would make any per
+ * CPU operation up to the point of the wrmsrl() fault.
+ *
+ * 64bit requires to point GSBASE to the new offset. Until the
+ * wrmsrl() happens the early mapping is still valid. That means
+ * the GSBASE update will lose any prior per CPU data which was
+ * not copied over in setup_per_cpu_areas().
+ *
+ * For secondary CPUs this is not a problem because they start
+ * already with the direct GDT and the real GSBASE. This invocation
+ * is pointless and will be removed in a subsequent step.
+ */
+ if (IS_ENABLED(CONFIG_X86_64))
+ wrmsrl(MSR_GS_BASE, cpu_kernelmode_gs_base(cpu));
}
static const struct cpu_dev *cpu_devs[X86_VENDOR_NUM] = {};
On Mon, Jul 18 2022 at 11:43, Linus Torvalds wrote:
> So I appreciate the added big comments in this code, but looking at this patch:
> On Mon, Jul 18, 2022 at 10:52 AM Thomas Gleixner <[email protected]> wrote:
>> + * For secondary CPUs this is not a problem because they start
>> + * already with the direct GDT and the real GSBASE. This invocation
>> + * is pointless and will be removed in a subsequent step.
>> + */
>> + if (IS_ENABLED(CONFIG_X86_64))
>> + wrmsrl(MSR_GS_BASE, cpu_kernelmode_gs_base(cpu));
>> }
>
> ... while those comments are nice and all, I do think this retains the
> basic insanity of having "switch_to_new_gdt()" do magical things on
> x86-64 that don't really match the name.
>
> So honestly, I'd be happier of that whole
>
> if (IS_ENABLED(CONFIG_X86_64))
> wrmsrl(MSR_GS_BASE, cpu_kernelmode_gs_base(cpu));
>
> was migrated to the callers instead. There aren't *that* many callers.
>
> I expect that it is then quite possible that several of the call-sites
> would go "GS_BASE is already correct here, I can remove this".
With the next patch we have only two left. The SMP and the UP case. Let
me look whether the UP needs it at all.
> But even if every single caller keeps that wrmsrl() around, at least
> it wouldn't be hidden behind a function call that has a name that
> implies something completely different is happening.
>
> And no, I don't care *that* deeply, so this is just a suggestion.
>
> But wouldn't it be nice if this function was actually named by what it
> does, rather than by what it used to do back in the i386 days when the
> GDT affected the segment bases?
Yes. Let me come up with a sensible name.
Thanks,
tglx
So I appreciate the added big comments in this code, but looking at this patch:
On Mon, Jul 18, 2022 at 10:52 AM Thomas Gleixner <[email protected]> wrote:
>
> +/**
> + * switch_to_new_gdt - Switch form early GDT to the direct one
> + * @cpu: The CPU number for which this is invoked
> + *
> + * Invoked during early boot to switch from early GDT and early per CPU
> + * (%fs on 32bit, GS_BASE on 64bit) to the direct GDT and the runtime per
> + * CPU area.
> */
> void switch_to_new_gdt(int cpu)
> {
> - /* Load the original GDT */
> load_direct_gdt(cpu);
> - /* Reload the per-cpu base */
> - load_percpu_segment(cpu);
> +
> + /*
> + * No need to load the %gs (%fs for 32bit) segment. It is already
> + * correct, %gs is 0 on 64bit and %fs is __KERNEL_PERCPU on 32 bit.
> + *
> + * Writing %gs on 64bit would zero GSBASE which would make any per
> + * CPU operation up to the point of the wrmsrl() fault.
> + *
> + * 64bit requires to point GSBASE to the new offset. Until the
> + * wrmsrl() happens the early mapping is still valid. That means
> + * the GSBASE update will lose any prior per CPU data which was
> + * not copied over in setup_per_cpu_areas().
> + *
> + * For secondary CPUs this is not a problem because they start
> + * already with the direct GDT and the real GSBASE. This invocation
> + * is pointless and will be removed in a subsequent step.
> + */
> + if (IS_ENABLED(CONFIG_X86_64))
> + wrmsrl(MSR_GS_BASE, cpu_kernelmode_gs_base(cpu));
> }
... while those comments are nice and all, I do think this retains the
basic insanity of having "switch_to_new_gdt()" do magical things on
x86-64 that don't really match the name.
So honestly, I'd be happier of that whole
if (IS_ENABLED(CONFIG_X86_64))
wrmsrl(MSR_GS_BASE, cpu_kernelmode_gs_base(cpu));
was migrated to the callers instead. There aren't *that* many callers.
I expect that it is then quite possible that several of the call-sites
would go "GS_BASE is already correct here, I can remove this".
But even if every single caller keeps that wrmsrl() around, at least
it wouldn't be hidden behind a function call that has a name that
implies something completely different is happening.
And no, I don't care *that* deeply, so this is just a suggestion.
But wouldn't it be nice if this function was actually named by what it
does, rather than by what it used to do back in the i386 days when the
GDT affected the segment bases?
Linus