* [PATCH -tip] x86/stackprotector: Move stack canary to struct pcpu_hot @ 2025-02-20 20:02 Uros Bizjak 2025-02-21 12:54 ` Brian Gerst 0 siblings, 1 reply; 11+ messages in thread From: Uros Bizjak @ 2025-02-20 20:02 UTC (permalink / raw) To: x86, linux-kernel Cc: Uros Bizjak, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Brian Gerst, Ard Biesheuvel Move stack canary from __stack_chk_guard to struct pcpu_hot and alias __stack_chk_guard to point to the new location in the linker script. __stack_chk_guard is one of the hottest data structures on x86, so moving it there makes sense even if its benefit cannot be measured explicitly. Signed-off-by: Uros Bizjak <ubizjak@gmail.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@kernel.org> Cc: Borislav Petkov <bp@alien8.de> Cc: Dave Hansen <dave.hansen@linux.intel.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Brian Gerst <brgerst@gmail.com> Cc: Ard Biesheuvel <ardb@kernel.org> --- arch/x86/include/asm/current.h | 13 +++++++++++++ arch/x86/kernel/cpu/common.c | 1 - arch/x86/kernel/vmlinux.lds.S | 2 ++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/arch/x86/include/asm/current.h b/arch/x86/include/asm/current.h index bf5953883ec3..e4ff1d15b465 100644 --- a/arch/x86/include/asm/current.h +++ b/arch/x86/include/asm/current.h @@ -15,6 +15,9 @@ struct task_struct; struct pcpu_hot { union { struct { +#ifdef CONFIG_STACKPROTECTOR + unsigned long stack_canary; +#endif struct task_struct *current_task; int preempt_count; int cpu_number; @@ -35,6 +38,16 @@ struct pcpu_hot { }; static_assert(sizeof(struct pcpu_hot) == 64); +/* + * stack_canary should be at the beginning of struct pcpu_hot to avoid: + * + * Invalid absolute R_X86_64_32S relocation: __stack_chk_guard + * + * error when aliasing __stack_chk_guard to struct pcpu_hot + * - see arch/x86/kernel/vmlinux.lds.S. + */ +static_assert(offsetof(struct pcpu_hot, stack_canary) == 0); + DECLARE_PER_CPU_ALIGNED(struct pcpu_hot, pcpu_hot); /* const-qualified alias to pcpu_hot, aliased by linker. */ diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index 21078907af57..9e54c1b585d2 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -2088,7 +2088,6 @@ void syscall_init(void) #endif /* CONFIG_X86_64 */ #ifdef CONFIG_STACKPROTECTOR -DEFINE_PER_CPU(unsigned long, __stack_chk_guard); #ifndef CONFIG_SMP EXPORT_PER_CPU_SYMBOL(__stack_chk_guard); #endif diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S index 1769a7126224..cabb86d505fc 100644 --- a/arch/x86/kernel/vmlinux.lds.S +++ b/arch/x86/kernel/vmlinux.lds.S @@ -467,6 +467,8 @@ SECTIONS . = ASSERT((_end - LOAD_OFFSET <= KERNEL_IMAGE_SIZE), "kernel image bigger than KERNEL_IMAGE_SIZE"); +PROVIDE(__stack_chk_guard = pcpu_hot); + /* needed for Clang - see arch/x86/entry/entry.S */ PROVIDE(__ref_stack_chk_guard = __stack_chk_guard); -- 2.42.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH -tip] x86/stackprotector: Move stack canary to struct pcpu_hot 2025-02-20 20:02 [PATCH -tip] x86/stackprotector: Move stack canary to struct pcpu_hot Uros Bizjak @ 2025-02-21 12:54 ` Brian Gerst 2025-02-21 13:24 ` Uros Bizjak 0 siblings, 1 reply; 11+ messages in thread From: Brian Gerst @ 2025-02-21 12:54 UTC (permalink / raw) To: Uros Bizjak Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Ard Biesheuvel On Thu, Feb 20, 2025 at 3:04 PM Uros Bizjak <ubizjak@gmail.com> wrote: > > Move stack canary from __stack_chk_guard to struct pcpu_hot and > alias __stack_chk_guard to point to the new location in the > linker script. > > __stack_chk_guard is one of the hottest data structures on x86, so > moving it there makes sense even if its benefit cannot be measured > explicitly. > > Signed-off-by: Uros Bizjak <ubizjak@gmail.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: Brian Gerst <brgerst@gmail.com> > Cc: Ard Biesheuvel <ardb@kernel.org> > --- > arch/x86/include/asm/current.h | 13 +++++++++++++ > arch/x86/kernel/cpu/common.c | 1 - > arch/x86/kernel/vmlinux.lds.S | 2 ++ > 3 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/current.h b/arch/x86/include/asm/current.h > index bf5953883ec3..e4ff1d15b465 100644 > --- a/arch/x86/include/asm/current.h > +++ b/arch/x86/include/asm/current.h > @@ -15,6 +15,9 @@ struct task_struct; > struct pcpu_hot { > union { > struct { > +#ifdef CONFIG_STACKPROTECTOR > + unsigned long stack_canary; > +#endif > struct task_struct *current_task; > int preempt_count; > int cpu_number; > @@ -35,6 +38,16 @@ struct pcpu_hot { > }; > static_assert(sizeof(struct pcpu_hot) == 64); > > +/* > + * stack_canary should be at the beginning of struct pcpu_hot to avoid: > + * > + * Invalid absolute R_X86_64_32S relocation: __stack_chk_guard This should be R_X86_64_PC32 relocations. > + * > + * error when aliasing __stack_chk_guard to struct pcpu_hot > + * - see arch/x86/kernel/vmlinux.lds.S. > + */ > +static_assert(offsetof(struct pcpu_hot, stack_canary) == 0); The simple solution to this is to add the symbol to the whitelist in tools/relocs.c: /* * These symbols are known to be relative, even if the linker marks them * as absolute (typically defined outside any section in the linker script.) */ I just got rid of hardcoding fixed_percpu_data from the start of percpu memory. I'd rather not add something similar back in. Brian Gerst ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -tip] x86/stackprotector: Move stack canary to struct pcpu_hot 2025-02-21 12:54 ` Brian Gerst @ 2025-02-21 13:24 ` Uros Bizjak 2025-02-21 13:36 ` Brian Gerst 0 siblings, 1 reply; 11+ messages in thread From: Uros Bizjak @ 2025-02-21 13:24 UTC (permalink / raw) To: Brian Gerst Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Ard Biesheuvel On Fri, Feb 21, 2025 at 1:54 PM Brian Gerst <brgerst@gmail.com> wrote: > > On Thu, Feb 20, 2025 at 3:04 PM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > Move stack canary from __stack_chk_guard to struct pcpu_hot and > > alias __stack_chk_guard to point to the new location in the > > linker script. > > > > __stack_chk_guard is one of the hottest data structures on x86, so > > moving it there makes sense even if its benefit cannot be measured > > explicitly. > > > > Signed-off-by: Uros Bizjak <ubizjak@gmail.com> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Ingo Molnar <mingo@kernel.org> > > Cc: Borislav Petkov <bp@alien8.de> > > Cc: Dave Hansen <dave.hansen@linux.intel.com> > > Cc: "H. Peter Anvin" <hpa@zytor.com> > > Cc: Brian Gerst <brgerst@gmail.com> > > Cc: Ard Biesheuvel <ardb@kernel.org> > > --- > > arch/x86/include/asm/current.h | 13 +++++++++++++ > > arch/x86/kernel/cpu/common.c | 1 - > > arch/x86/kernel/vmlinux.lds.S | 2 ++ > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/include/asm/current.h b/arch/x86/include/asm/current.h > > index bf5953883ec3..e4ff1d15b465 100644 > > --- a/arch/x86/include/asm/current.h > > +++ b/arch/x86/include/asm/current.h > > @@ -15,6 +15,9 @@ struct task_struct; > > struct pcpu_hot { > > union { > > struct { > > +#ifdef CONFIG_STACKPROTECTOR > > + unsigned long stack_canary; > > +#endif > > struct task_struct *current_task; > > int preempt_count; > > int cpu_number; > > @@ -35,6 +38,16 @@ struct pcpu_hot { > > }; > > static_assert(sizeof(struct pcpu_hot) == 64); > > > > +/* > > + * stack_canary should be at the beginning of struct pcpu_hot to avoid: > > + * > > + * Invalid absolute R_X86_64_32S relocation: __stack_chk_guard > > This should be R_X86_64_PC32 relocations. This is what the build system reports if any offset (including 0) is added to PROVIDE(__stack_chk_guard = pcpu_hot); It is not a warning, but an error that fails the build. As was discussed in the previous thread, the above is required to handle !SMP case, where mstack-protector-guard=global (used by !SMP builds) ignores the -mstack-protector-guard-symbol option and always uses __stack_chk_guard. Uros. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -tip] x86/stackprotector: Move stack canary to struct pcpu_hot 2025-02-21 13:24 ` Uros Bizjak @ 2025-02-21 13:36 ` Brian Gerst 2025-02-21 14:02 ` Uros Bizjak 0 siblings, 1 reply; 11+ messages in thread From: Brian Gerst @ 2025-02-21 13:36 UTC (permalink / raw) To: Uros Bizjak Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Ard Biesheuvel [-- Attachment #1: Type: text/plain, Size: 2801 bytes --] On Fri, Feb 21, 2025 at 8:25 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > On Fri, Feb 21, 2025 at 1:54 PM Brian Gerst <brgerst@gmail.com> wrote: > > > > On Thu, Feb 20, 2025 at 3:04 PM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > > > Move stack canary from __stack_chk_guard to struct pcpu_hot and > > > alias __stack_chk_guard to point to the new location in the > > > linker script. > > > > > > __stack_chk_guard is one of the hottest data structures on x86, so > > > moving it there makes sense even if its benefit cannot be measured > > > explicitly. > > > > > > Signed-off-by: Uros Bizjak <ubizjak@gmail.com> > > > Cc: Thomas Gleixner <tglx@linutronix.de> > > > Cc: Ingo Molnar <mingo@kernel.org> > > > Cc: Borislav Petkov <bp@alien8.de> > > > Cc: Dave Hansen <dave.hansen@linux.intel.com> > > > Cc: "H. Peter Anvin" <hpa@zytor.com> > > > Cc: Brian Gerst <brgerst@gmail.com> > > > Cc: Ard Biesheuvel <ardb@kernel.org> > > > --- > > > arch/x86/include/asm/current.h | 13 +++++++++++++ > > > arch/x86/kernel/cpu/common.c | 1 - > > > arch/x86/kernel/vmlinux.lds.S | 2 ++ > > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/include/asm/current.h b/arch/x86/include/asm/current.h > > > index bf5953883ec3..e4ff1d15b465 100644 > > > --- a/arch/x86/include/asm/current.h > > > +++ b/arch/x86/include/asm/current.h > > > @@ -15,6 +15,9 @@ struct task_struct; > > > struct pcpu_hot { > > > union { > > > struct { > > > +#ifdef CONFIG_STACKPROTECTOR > > > + unsigned long stack_canary; > > > +#endif > > > struct task_struct *current_task; > > > int preempt_count; > > > int cpu_number; > > > @@ -35,6 +38,16 @@ struct pcpu_hot { > > > }; > > > static_assert(sizeof(struct pcpu_hot) == 64); > > > > > > +/* > > > + * stack_canary should be at the beginning of struct pcpu_hot to avoid: > > > + * > > > + * Invalid absolute R_X86_64_32S relocation: __stack_chk_guard > > > > This should be R_X86_64_PC32 relocations. > > This is what the build system reports if any offset (including 0) is added to > > PROVIDE(__stack_chk_guard = pcpu_hot); > > It is not a warning, but an error that fails the build. > > As was discussed in the previous thread, the above is required to > handle !SMP case, where mstack-protector-guard=global (used by !SMP > builds) ignores the > -mstack-protector-guard-symbol option and always uses __stack_chk_guard. I got a warning from the relocs tool, but not a hard error. What compiler/linker are you using? Does the attached patch build in your configuration? Brian Gerst [-- Attachment #2: 0001-x86-stackprotector-Move-canary-to-pcpu_hot.patch --] [-- Type: text/x-patch, Size: 7413 bytes --] From ee30f3a936e641978c965d82990c11792b598fd8 Mon Sep 17 00:00:00 2001 From: Brian Gerst <brgerst@gmail.com> Date: Wed, 19 Feb 2025 12:38:02 -0500 Subject: [PATCH] x86/stackprotector: Move canary to pcpu_hot The stack protector canary value is frequently accessed on function entry and exit, so move it to the pcpu_hot struct where it will have fast access. In order to keep non-SMP builds working, rename __ref_stack_chk_guard to __stack_chk_guard. Signed-off-by: Brian Gerst <brgerst@gmail.com> --- arch/x86/Makefile | 2 +- arch/x86/entry/entry.S | 2 +- arch/x86/entry/entry_32.S | 2 +- arch/x86/entry/entry_64.S | 2 +- arch/x86/include/asm/asm-prototypes.h | 2 +- arch/x86/include/asm/current.h | 3 +++ arch/x86/include/asm/stackprotector.h | 7 +++---- arch/x86/kernel/asm-offsets.c | 4 ++++ arch/x86/kernel/cpu/common.c | 8 -------- arch/x86/kernel/module.c | 2 +- arch/x86/kernel/vmlinux.lds.S | 4 +++- arch/x86/tools/relocs.c | 1 + 12 files changed, 20 insertions(+), 19 deletions(-) diff --git a/arch/x86/Makefile b/arch/x86/Makefile index 88a1705366f9..cc619d31ccfe 100644 --- a/arch/x86/Makefile +++ b/arch/x86/Makefile @@ -197,7 +197,7 @@ endif ifeq ($(CONFIG_STACKPROTECTOR),y) ifeq ($(CONFIG_SMP),y) KBUILD_CFLAGS += -mstack-protector-guard-reg=$(percpu_seg) - KBUILD_CFLAGS += -mstack-protector-guard-symbol=__ref_stack_chk_guard + KBUILD_CFLAGS += -mstack-protector-guard-symbol=__stack_chk_guard else KBUILD_CFLAGS += -mstack-protector-guard=global endif diff --git a/arch/x86/entry/entry.S b/arch/x86/entry/entry.S index fe5344a249a1..d779381a067c 100644 --- a/arch/x86/entry/entry.S +++ b/arch/x86/entry/entry.S @@ -63,5 +63,5 @@ THUNK warn_thunk_thunk, __warn_thunk * instead. */ #ifdef CONFIG_STACKPROTECTOR -EXPORT_SYMBOL(__ref_stack_chk_guard); +EXPORT_SYMBOL(__stack_chk_guard); #endif diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S index 20be5758c2d2..940efc07f2c1 100644 --- a/arch/x86/entry/entry_32.S +++ b/arch/x86/entry/entry_32.S @@ -691,7 +691,7 @@ SYM_CODE_START(__switch_to_asm) #ifdef CONFIG_STACKPROTECTOR movl TASK_stack_canary(%edx), %ebx - movl %ebx, PER_CPU_VAR(__stack_chk_guard) + movl %ebx, PER_CPU_VAR(pcpu_hot + X86_stack_canary) #endif /* diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index 33a955aa01d8..ed43559d66dc 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -192,7 +192,7 @@ SYM_FUNC_START(__switch_to_asm) #ifdef CONFIG_STACKPROTECTOR movq TASK_stack_canary(%rsi), %rbx - movq %rbx, PER_CPU_VAR(__stack_chk_guard) + movq %rbx, PER_CPU_VAR(pcpu_hot + X86_stack_canary) #endif /* diff --git a/arch/x86/include/asm/asm-prototypes.h b/arch/x86/include/asm/asm-prototypes.h index 3674006e3974..43977c74b22f 100644 --- a/arch/x86/include/asm/asm-prototypes.h +++ b/arch/x86/include/asm/asm-prototypes.h @@ -21,5 +21,5 @@ extern void cmpxchg8b_emu(void); #endif #if defined(__GENKSYMS__) && defined(CONFIG_STACKPROTECTOR) -extern unsigned long __ref_stack_chk_guard; +extern unsigned long __stack_chk_guard; #endif diff --git a/arch/x86/include/asm/current.h b/arch/x86/include/asm/current.h index bf5953883ec3..cf0ce44c1d95 100644 --- a/arch/x86/include/asm/current.h +++ b/arch/x86/include/asm/current.h @@ -28,6 +28,9 @@ struct pcpu_hot { bool hardirq_stack_inuse; #else void *softirq_stack_ptr; +#endif +#ifdef CONFIG_STACKPROTECTOR + unsigned long stack_canary; #endif }; u8 pad[64]; diff --git a/arch/x86/include/asm/stackprotector.h b/arch/x86/include/asm/stackprotector.h index d43fb589fcf6..4142c88390d3 100644 --- a/arch/x86/include/asm/stackprotector.h +++ b/arch/x86/include/asm/stackprotector.h @@ -17,11 +17,10 @@ #include <asm/processor.h> #include <asm/percpu.h> #include <asm/desc.h> +#include <asm/current.h> #include <linux/sched.h> -DECLARE_PER_CPU(unsigned long, __stack_chk_guard); - /* * Initialize the stackprotector canary value. * @@ -38,12 +37,12 @@ static __always_inline void boot_init_stack_canary(void) unsigned long canary = get_random_canary(); current->stack_canary = canary; - this_cpu_write(__stack_chk_guard, canary); + this_cpu_write(pcpu_hot.stack_canary, canary); } static inline void cpu_init_stack_canary(int cpu, struct task_struct *idle) { - per_cpu(__stack_chk_guard, cpu) = idle->stack_canary; + per_cpu(pcpu_hot.stack_canary, cpu) = idle->stack_canary; } #else /* STACKPROTECTOR */ diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c index a98020bf31bb..4c02f5d247f2 100644 --- a/arch/x86/kernel/asm-offsets.c +++ b/arch/x86/kernel/asm-offsets.c @@ -112,6 +112,10 @@ static void __used common(void) #ifdef CONFIG_MITIGATION_CALL_DEPTH_TRACKING OFFSET(X86_call_depth, pcpu_hot, call_depth); #endif +#ifdef CONFIG_STACKPROTECTOR + OFFSET(X86_stack_canary, pcpu_hot, stack_canary); +#endif + #if IS_ENABLED(CONFIG_CRYPTO_ARIA_AESNI_AVX_X86_64) /* Offset for fields in aria_ctx */ BLANK(); diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index 8b49b1338f76..1735e4a11d9e 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -24,7 +24,6 @@ #include <linux/io.h> #include <linux/syscore_ops.h> #include <linux/pgtable.h> -#include <linux/stackprotector.h> #include <linux/utsname.h> #include <asm/alternative.h> @@ -2087,13 +2086,6 @@ void syscall_init(void) } #endif /* CONFIG_X86_64 */ -#ifdef CONFIG_STACKPROTECTOR -DEFINE_PER_CPU(unsigned long, __stack_chk_guard); -#ifndef CONFIG_SMP -EXPORT_PER_CPU_SYMBOL(__stack_chk_guard); -#endif -#endif - /* * Clear all 6 debug registers: */ diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c index a286f32c5503..fbadc78957df 100644 --- a/arch/x86/kernel/module.c +++ b/arch/x86/kernel/module.c @@ -134,7 +134,7 @@ static int __write_relocate_add(Elf64_Shdr *sechdrs, #if defined(CONFIG_STACKPROTECTOR) && \ defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 170000 case R_X86_64_REX_GOTPCRELX: { - static unsigned long __percpu *const addr = &__stack_chk_guard; + static unsigned long __percpu *const addr = &pcpu_hot.stack_canary; if (sym->st_value != (u64)addr) { pr_err("%s: Unsupported GOTPCREL relocation\n", me->name); diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S index 1769a7126224..cb0ab873616e 100644 --- a/arch/x86/kernel/vmlinux.lds.S +++ b/arch/x86/kernel/vmlinux.lds.S @@ -467,8 +467,10 @@ SECTIONS . = ASSERT((_end - LOAD_OFFSET <= KERNEL_IMAGE_SIZE), "kernel image bigger than KERNEL_IMAGE_SIZE"); +#ifdef CONFIG_STACKPROTECTOR /* needed for Clang - see arch/x86/entry/entry.S */ -PROVIDE(__ref_stack_chk_guard = __stack_chk_guard); +__stack_chk_guard = pcpu_hot + X86_stack_canary; +#endif #ifdef CONFIG_X86_64 diff --git a/arch/x86/tools/relocs.c b/arch/x86/tools/relocs.c index 5778bc498415..bf612f1f8c8a 100644 --- a/arch/x86/tools/relocs.c +++ b/arch/x86/tools/relocs.c @@ -89,6 +89,7 @@ static const char * const sym_regex_kernel[S_NSYMTYPES] = { "__end_rodata_aligned|" "__initramfs_start|" "(jiffies|jiffies_64)|" + "__stack_chk_guard|" #if ELF_BITS == 64 "__end_rodata_hpage_align|" #endif base-commit: 01157ddc58dc2fe428ec17dd5a18cc13f134639f -- 2.48.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH -tip] x86/stackprotector: Move stack canary to struct pcpu_hot 2025-02-21 13:36 ` Brian Gerst @ 2025-02-21 14:02 ` Uros Bizjak 2025-02-21 14:13 ` Ard Biesheuvel 0 siblings, 1 reply; 11+ messages in thread From: Uros Bizjak @ 2025-02-21 14:02 UTC (permalink / raw) To: Brian Gerst Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Ard Biesheuvel On Fri, Feb 21, 2025 at 2:37 PM Brian Gerst <brgerst@gmail.com> wrote: > > On Fri, Feb 21, 2025 at 8:25 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > On Fri, Feb 21, 2025 at 1:54 PM Brian Gerst <brgerst@gmail.com> wrote: > > > > > > On Thu, Feb 20, 2025 at 3:04 PM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > > > > > Move stack canary from __stack_chk_guard to struct pcpu_hot and > > > > alias __stack_chk_guard to point to the new location in the > > > > linker script. > > > > > > > > __stack_chk_guard is one of the hottest data structures on x86, so > > > > moving it there makes sense even if its benefit cannot be measured > > > > explicitly. > > > > > > > > Signed-off-by: Uros Bizjak <ubizjak@gmail.com> > > > > Cc: Thomas Gleixner <tglx@linutronix.de> > > > > Cc: Ingo Molnar <mingo@kernel.org> > > > > Cc: Borislav Petkov <bp@alien8.de> > > > > Cc: Dave Hansen <dave.hansen@linux.intel.com> > > > > Cc: "H. Peter Anvin" <hpa@zytor.com> > > > > Cc: Brian Gerst <brgerst@gmail.com> > > > > Cc: Ard Biesheuvel <ardb@kernel.org> > > > > --- > > > > arch/x86/include/asm/current.h | 13 +++++++++++++ > > > > arch/x86/kernel/cpu/common.c | 1 - > > > > arch/x86/kernel/vmlinux.lds.S | 2 ++ > > > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/x86/include/asm/current.h b/arch/x86/include/asm/current.h > > > > index bf5953883ec3..e4ff1d15b465 100644 > > > > --- a/arch/x86/include/asm/current.h > > > > +++ b/arch/x86/include/asm/current.h > > > > @@ -15,6 +15,9 @@ struct task_struct; > > > > struct pcpu_hot { > > > > union { > > > > struct { > > > > +#ifdef CONFIG_STACKPROTECTOR > > > > + unsigned long stack_canary; > > > > +#endif > > > > struct task_struct *current_task; > > > > int preempt_count; > > > > int cpu_number; > > > > @@ -35,6 +38,16 @@ struct pcpu_hot { > > > > }; > > > > static_assert(sizeof(struct pcpu_hot) == 64); > > > > > > > > +/* > > > > + * stack_canary should be at the beginning of struct pcpu_hot to avoid: > > > > + * > > > > + * Invalid absolute R_X86_64_32S relocation: __stack_chk_guard > > > > > > This should be R_X86_64_PC32 relocations. > > > > This is what the build system reports if any offset (including 0) is added to > > > > PROVIDE(__stack_chk_guard = pcpu_hot); > > > > It is not a warning, but an error that fails the build. > > > > As was discussed in the previous thread, the above is required to > > handle !SMP case, where mstack-protector-guard=global (used by !SMP > > builds) ignores the > > -mstack-protector-guard-symbol option and always uses __stack_chk_guard. > > I got a warning from the relocs tool, but not a hard error. What > compiler/linker are you using? > > Does the attached patch build in your configuration? Ah, the attached patch is similar to my previous approach, where the build system *warned* on an offset (the patch was abandoned due to Ard's request to put stack_canary to the *beginning* of struct pcpu_hot, and this allowed for a simplified patch). The attached patch builds for me without warning/error for both, SMP and !SMP build. You can put my Acked-by: on the patch, and if it is based on my previous patch, I'd be grateful for a Co-developed-by tag. Thanks. Uros. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -tip] x86/stackprotector: Move stack canary to struct pcpu_hot 2025-02-21 14:02 ` Uros Bizjak @ 2025-02-21 14:13 ` Ard Biesheuvel 2025-02-21 14:33 ` Uros Bizjak 0 siblings, 1 reply; 11+ messages in thread From: Ard Biesheuvel @ 2025-02-21 14:13 UTC (permalink / raw) To: Uros Bizjak Cc: Brian Gerst, x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin On Fri, 21 Feb 2025 at 15:02, Uros Bizjak <ubizjak@gmail.com> wrote: > > On Fri, Feb 21, 2025 at 2:37 PM Brian Gerst <brgerst@gmail.com> wrote: > > > > On Fri, Feb 21, 2025 at 8:25 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > > > On Fri, Feb 21, 2025 at 1:54 PM Brian Gerst <brgerst@gmail.com> wrote: > > > > > > > > On Thu, Feb 20, 2025 at 3:04 PM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > > > > > > > Move stack canary from __stack_chk_guard to struct pcpu_hot and > > > > > alias __stack_chk_guard to point to the new location in the > > > > > linker script. > > > > > > > > > > __stack_chk_guard is one of the hottest data structures on x86, so > > > > > moving it there makes sense even if its benefit cannot be measured > > > > > explicitly. > > > > > > > > > > Signed-off-by: Uros Bizjak <ubizjak@gmail.com> > > > > > Cc: Thomas Gleixner <tglx@linutronix.de> > > > > > Cc: Ingo Molnar <mingo@kernel.org> > > > > > Cc: Borislav Petkov <bp@alien8.de> > > > > > Cc: Dave Hansen <dave.hansen@linux.intel.com> > > > > > Cc: "H. Peter Anvin" <hpa@zytor.com> > > > > > Cc: Brian Gerst <brgerst@gmail.com> > > > > > Cc: Ard Biesheuvel <ardb@kernel.org> > > > > > --- > > > > > arch/x86/include/asm/current.h | 13 +++++++++++++ > > > > > arch/x86/kernel/cpu/common.c | 1 - > > > > > arch/x86/kernel/vmlinux.lds.S | 2 ++ > > > > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/arch/x86/include/asm/current.h b/arch/x86/include/asm/current.h > > > > > index bf5953883ec3..e4ff1d15b465 100644 > > > > > --- a/arch/x86/include/asm/current.h > > > > > +++ b/arch/x86/include/asm/current.h > > > > > @@ -15,6 +15,9 @@ struct task_struct; > > > > > struct pcpu_hot { > > > > > union { > > > > > struct { > > > > > +#ifdef CONFIG_STACKPROTECTOR > > > > > + unsigned long stack_canary; > > > > > +#endif > > > > > struct task_struct *current_task; > > > > > int preempt_count; > > > > > int cpu_number; > > > > > @@ -35,6 +38,16 @@ struct pcpu_hot { > > > > > }; > > > > > static_assert(sizeof(struct pcpu_hot) == 64); > > > > > > > > > > +/* > > > > > + * stack_canary should be at the beginning of struct pcpu_hot to avoid: > > > > > + * > > > > > + * Invalid absolute R_X86_64_32S relocation: __stack_chk_guard > > > > > > > > This should be R_X86_64_PC32 relocations. > > > > > > This is what the build system reports if any offset (including 0) is added to > > > > > > PROVIDE(__stack_chk_guard = pcpu_hot); > > > > > > It is not a warning, but an error that fails the build. > > > > > > As was discussed in the previous thread, the above is required to > > > handle !SMP case, where mstack-protector-guard=global (used by !SMP > > > builds) ignores the > > > -mstack-protector-guard-symbol option and always uses __stack_chk_guard. > > > > I got a warning from the relocs tool, but not a hard error. What > > compiler/linker are you using? > > > > Does the attached patch build in your configuration? > > Ah, the attached patch is similar to my previous approach, where the > build system *warned* on an offset (the patch was abandoned due to > Ard's request to put stack_canary to the *beginning* of struct > pcpu_hot, and this allowed for a simplified patch). > > The attached patch builds for me without warning/error for both, SMP > and !SMP build. > Did you try building modules too? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -tip] x86/stackprotector: Move stack canary to struct pcpu_hot 2025-02-21 14:13 ` Ard Biesheuvel @ 2025-02-21 14:33 ` Uros Bizjak 2025-02-21 14:38 ` Ard Biesheuvel 0 siblings, 1 reply; 11+ messages in thread From: Uros Bizjak @ 2025-02-21 14:33 UTC (permalink / raw) To: Ard Biesheuvel Cc: Brian Gerst, x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin On Fri, Feb 21, 2025 at 3:13 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > I got a warning from the relocs tool, but not a hard error. What > > > compiler/linker are you using? > > > > > > Does the attached patch build in your configuration? > > > > Ah, the attached patch is similar to my previous approach, where the > > build system *warned* on an offset (the patch was abandoned due to > > Ard's request to put stack_canary to the *beginning* of struct > > pcpu_hot, and this allowed for a simplified patch). > > > > The attached patch builds for me without warning/error for both, SMP > > and !SMP build. > > > > Did you try building modules too? make -j 24 olddefconfig prepare modules_prepare bzImage modules for defconfig, SMP and !SMP. Uros. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -tip] x86/stackprotector: Move stack canary to struct pcpu_hot 2025-02-21 14:33 ` Uros Bizjak @ 2025-02-21 14:38 ` Ard Biesheuvel 2025-02-21 15:54 ` Brian Gerst 0 siblings, 1 reply; 11+ messages in thread From: Ard Biesheuvel @ 2025-02-21 14:38 UTC (permalink / raw) To: Uros Bizjak Cc: Brian Gerst, x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin On Fri, 21 Feb 2025 at 15:33, Uros Bizjak <ubizjak@gmail.com> wrote: > > On Fri, Feb 21, 2025 at 3:13 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > I got a warning from the relocs tool, but not a hard error. What > > > > compiler/linker are you using? > > > > > > > > Does the attached patch build in your configuration? > > > > > > Ah, the attached patch is similar to my previous approach, where the > > > build system *warned* on an offset (the patch was abandoned due to > > > Ard's request to put stack_canary to the *beginning* of struct > > > pcpu_hot, and this allowed for a simplified patch). > > > > > > The attached patch builds for me without warning/error for both, SMP > > > and !SMP build. > > > > > > > Did you try building modules too? > > make -j 24 olddefconfig prepare modules_prepare bzImage modules > > for defconfig, SMP and !SMP. > OK. I think I prefer Brian's approach - the only nit is that the placement of stack_canary creates a padding hole in struct pcpu_hot. However, that does not actually matter until we run out of space so I think it is fine as is. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -tip] x86/stackprotector: Move stack canary to struct pcpu_hot 2025-02-21 14:38 ` Ard Biesheuvel @ 2025-02-21 15:54 ` Brian Gerst 2025-02-21 16:01 ` Uros Bizjak 0 siblings, 1 reply; 11+ messages in thread From: Brian Gerst @ 2025-02-21 15:54 UTC (permalink / raw) To: Ard Biesheuvel Cc: Uros Bizjak, x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin On Fri, Feb 21, 2025 at 9:38 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Fri, 21 Feb 2025 at 15:33, Uros Bizjak <ubizjak@gmail.com> wrote: > > > > On Fri, Feb 21, 2025 at 3:13 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > > I got a warning from the relocs tool, but not a hard error. What > > > > > compiler/linker are you using? > > > > > > > > > > Does the attached patch build in your configuration? > > > > > > > > Ah, the attached patch is similar to my previous approach, where the > > > > build system *warned* on an offset (the patch was abandoned due to > > > > Ard's request to put stack_canary to the *beginning* of struct > > > > pcpu_hot, and this allowed for a simplified patch). > > > > > > > > The attached patch builds for me without warning/error for both, SMP > > > > and !SMP build. > > > > > > > > > > Did you try building modules too? > > > > make -j 24 olddefconfig prepare modules_prepare bzImage modules > > > > for defconfig, SMP and !SMP. > > > > OK. > > I think I prefer Brian's approach - the only nit is that the placement > of stack_canary creates a padding hole in struct pcpu_hot. However, > that does not actually matter until we run out of space so I think it > is fine as is. Going off on a tangent, as struct pcpu_hot grows I think it can be done better as a subsection of percpu data (".data..percpu..hot"). That way each variable has its own symbol again and it reduces header dependencies. Brian Gerst ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -tip] x86/stackprotector: Move stack canary to struct pcpu_hot 2025-02-21 15:54 ` Brian Gerst @ 2025-02-21 16:01 ` Uros Bizjak 2025-02-21 16:47 ` Brian Gerst 0 siblings, 1 reply; 11+ messages in thread From: Uros Bizjak @ 2025-02-21 16:01 UTC (permalink / raw) To: Brian Gerst Cc: Ard Biesheuvel, x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin On Fri, Feb 21, 2025 at 4:54 PM Brian Gerst <brgerst@gmail.com> wrote: > > On Fri, Feb 21, 2025 at 9:38 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Fri, 21 Feb 2025 at 15:33, Uros Bizjak <ubizjak@gmail.com> wrote: > > > > > > On Fri, Feb 21, 2025 at 3:13 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > > > > I got a warning from the relocs tool, but not a hard error. What > > > > > > compiler/linker are you using? > > > > > > > > > > > > Does the attached patch build in your configuration? > > > > > > > > > > Ah, the attached patch is similar to my previous approach, where the > > > > > build system *warned* on an offset (the patch was abandoned due to > > > > > Ard's request to put stack_canary to the *beginning* of struct > > > > > pcpu_hot, and this allowed for a simplified patch). > > > > > > > > > > The attached patch builds for me without warning/error for both, SMP > > > > > and !SMP build. > > > > > > > > > > > > > Did you try building modules too? > > > > > > make -j 24 olddefconfig prepare modules_prepare bzImage modules > > > > > > for defconfig, SMP and !SMP. > > > > > > > OK. > > > > I think I prefer Brian's approach - the only nit is that the placement > > of stack_canary creates a padding hole in struct pcpu_hot. However, > > that does not actually matter until we run out of space so I think it > > is fine as is. > > Going off on a tangent, as struct pcpu_hot grows I think it can be > done better as a subsection of percpu data (".data..percpu..hot"). > That way each variable has its own symbol again and it reduces header > dependencies. Please note an optimization in common.h: /* const-qualified alias to pcpu_hot, aliased by linker. */ DECLARE_PER_CPU_ALIGNED(const struct pcpu_hot __percpu_seg_override, const_pcpu_hot); struct pcpu_hot has its const alias, so we are able to use: if (IS_ENABLED(CONFIG_USE_X86_SEG_SUPPORT)) return this_cpu_read_const(const_pcpu_hot.current_task); where the compiler is able to eliminate a sequence of reads to the const_pcpu_hot.current_task. LTO does not like the approach, though, but clang does not use x86 seg support, and gcc does not perform LTO. Uros. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -tip] x86/stackprotector: Move stack canary to struct pcpu_hot 2025-02-21 16:01 ` Uros Bizjak @ 2025-02-21 16:47 ` Brian Gerst 0 siblings, 0 replies; 11+ messages in thread From: Brian Gerst @ 2025-02-21 16:47 UTC (permalink / raw) To: Uros Bizjak Cc: Ard Biesheuvel, x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin On Fri, Feb 21, 2025 at 11:02 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > On Fri, Feb 21, 2025 at 4:54 PM Brian Gerst <brgerst@gmail.com> wrote: > > > > On Fri, Feb 21, 2025 at 9:38 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > On Fri, 21 Feb 2025 at 15:33, Uros Bizjak <ubizjak@gmail.com> wrote: > > > > > > > > On Fri, Feb 21, 2025 at 3:13 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > > > > > > I got a warning from the relocs tool, but not a hard error. What > > > > > > > compiler/linker are you using? > > > > > > > > > > > > > > Does the attached patch build in your configuration? > > > > > > > > > > > > Ah, the attached patch is similar to my previous approach, where the > > > > > > build system *warned* on an offset (the patch was abandoned due to > > > > > > Ard's request to put stack_canary to the *beginning* of struct > > > > > > pcpu_hot, and this allowed for a simplified patch). > > > > > > > > > > > > The attached patch builds for me without warning/error for both, SMP > > > > > > and !SMP build. > > > > > > > > > > > > > > > > Did you try building modules too? > > > > > > > > make -j 24 olddefconfig prepare modules_prepare bzImage modules > > > > > > > > for defconfig, SMP and !SMP. > > > > > > > > > > OK. > > > > > > I think I prefer Brian's approach - the only nit is that the placement > > > of stack_canary creates a padding hole in struct pcpu_hot. However, > > > that does not actually matter until we run out of space so I think it > > > is fine as is. > > > > Going off on a tangent, as struct pcpu_hot grows I think it can be > > done better as a subsection of percpu data (".data..percpu..hot"). > > That way each variable has its own symbol again and it reduces header > > dependencies. > > Please note an optimization in common.h: > > /* const-qualified alias to pcpu_hot, aliased by linker. */ > DECLARE_PER_CPU_ALIGNED(const struct pcpu_hot __percpu_seg_override, > const_pcpu_hot); > > struct pcpu_hot has its const alias, so we are able to use: > > if (IS_ENABLED(CONFIG_USE_X86_SEG_SUPPORT)) > return this_cpu_read_const(const_pcpu_hot.current_task); > > where the compiler is able to eliminate a sequence of reads to the > const_pcpu_hot.current_task. > > LTO does not like the approach, though, but clang does not use x86 seg > support, and gcc does not perform LTO. Noted. I'll look into this over the weekend. Brian Gerst ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-02-21 16:48 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-02-20 20:02 [PATCH -tip] x86/stackprotector: Move stack canary to struct pcpu_hot Uros Bizjak 2025-02-21 12:54 ` Brian Gerst 2025-02-21 13:24 ` Uros Bizjak 2025-02-21 13:36 ` Brian Gerst 2025-02-21 14:02 ` Uros Bizjak 2025-02-21 14:13 ` Ard Biesheuvel 2025-02-21 14:33 ` Uros Bizjak 2025-02-21 14:38 ` Ard Biesheuvel 2025-02-21 15:54 ` Brian Gerst 2025-02-21 16:01 ` Uros Bizjak 2025-02-21 16:47 ` Brian Gerst
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox