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(-)
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.0On 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
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.
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
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.
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?
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.
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.
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
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.
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
© 2016 - 2025 Red Hat, Inc.