arch/x86/Kconfig.assembler | 1 + 1 file changed, 1 insertion(+)
Since commit 18e66b695e78 ("x86/shstk: Add Kconfig option for shadow
stack"), AS_WRUSS is set even in 32-bit .configs. It is due to how
Kbuild works. .config is not considered during make oldconfig (and other
make *config), so standard (64-bit) gcc is invoked from 'as-instr'
Kbuild tests. And such gcc indeed reports that wruss is supported, so
AS_WRUSS=y is set.
Provided the wruss instruction is 64-bit only (and used in pure 64-bit
X86_USER_SHADOW_STACK), it has little sense to have AS_WRUSS=y set on
32-bit.
Therefore, make the whole test dependent on X86_64 to ensure it's set
only on 64-bit.
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Borislav Petkov (AMD) <bp@alien8.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: Mike Rapoport (IBM) <rppt@kernel.org>
Cc: Pengfei Xu <pengfei.xu@intel.com>
Cc: John Allen <john.allen@amd.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
---
arch/x86/Kconfig.assembler | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/Kconfig.assembler b/arch/x86/Kconfig.assembler
index 8ad41da301e5..a5b5241711e3 100644
--- a/arch/x86/Kconfig.assembler
+++ b/arch/x86/Kconfig.assembler
@@ -27,5 +27,6 @@ config AS_GFNI
config AS_WRUSS
def_bool $(as-instr,wrussq %rax$(comma)(%rbx))
+ depends on X86_64
help
Supported by binutils >= 2.31 and LLVM integrated assembler
--
2.42.0On 10/31/23 03:21, Jiri Slaby (SUSE) wrote:
...
> Provided the wruss instruction is 64-bit only (and used in pure 64-bit
> X86_USER_SHADOW_STACK), it has little sense to have AS_WRUSS=y set on
> 32-bit.
>
> Therefore, make the whole test dependent on X86_64 to ensure it's set
> only on 64-bit.
...
> config AS_WRUSS
> def_bool $(as-instr,wrussq %rax$(comma)(%rbx))
> + depends on X86_64
> help
> Supported by binutils >= 2.31 and LLVM integrated assembler
What's the downside to just leaving this alone?
This patch just seems wrong logically. Suppose some deranged person
wanted 32-bit shadow stack support. They'd have to go hunt this down
via trial and error instead of just enabling X86_USER_SHADOW_STACK.
Granted, that would take one crazy person five minutes to figure out why
their .config is broken, but it still seems wrong. It's especially
wrong without a comment because it logically reads something along the
lines of "WRUSS is only available on x86_64 configs".
A better way to do this would be:
config HAS_SHADOW_STACKS
depends on X86_64
config AS_WRUSS
...
# Avoid setting AS_WRUSS on configs that don't need it:
depends on HAS_SHADOW_STACKS
config X86_USER_SHADOW_STACK
bool "X86 userspace shadow stack"
depends on AS_WRUSS
depends on HAS_SHADOW_STACKS
But that honestly doesn't seem worth it because (circling back to the
first thing I wrote...) I don't really know what the benefit is to doing
this in the first place.
On 31. 10. 23, 14:53, Dave Hansen wrote: > On 10/31/23 03:21, Jiri Slaby (SUSE) wrote: > ... >> Provided the wruss instruction is 64-bit only (and used in pure 64-bit >> X86_USER_SHADOW_STACK), it has little sense to have AS_WRUSS=y set on >> 32-bit. >> >> Therefore, make the whole test dependent on X86_64 to ensure it's set >> only on 64-bit. > ... >> config AS_WRUSS >> def_bool $(as-instr,wrussq %rax$(comma)(%rbx)) >> + depends on X86_64 >> help >> Supported by binutils >= 2.31 and LLVM integrated assembler > > What's the downside to just leaving this alone? > > This patch just seems wrong logically. Suppose some deranged person > wanted 32-bit shadow stack support. They'd have to go hunt this down > via trial and error instead of just enabling X86_USER_SHADOW_STACK. All wrussq, rax, rbx can never be right on 32-bit anyway... > Granted, that would take one crazy person five minutes to figure out why > their .config is broken, but it still seems wrong. It's especially > wrong without a comment because it logically reads something along the > lines of "WRUSS is only available on x86_64 configs". Which is right, or what am I missing? > A better way to do this would be: > > config HAS_SHADOW_STACKS > depends on X86_64 > > config AS_WRUSS > ... > # Avoid setting AS_WRUSS on configs that don't need it: > depends on HAS_SHADOW_STACKS > > config X86_USER_SHADOW_STACK > bool "X86 userspace shadow stack" > depends on AS_WRUSS > depends on HAS_SHADOW_STACKS > > But that honestly doesn't seem worth it because (circling back to the > first thing I wrote...) I don't really know what the benefit is to doing > this in the first place. Again, to avoid nonsense in 32bit configs produced by oldconfig (as I noted in the commit log). thanks, -- js suse labs
+ linux-kbuild@vger.kernel.org
On Tue, Oct 31, 2023 at 11:21:11AM +0100, Jiri Slaby (SUSE) wrote:
> Since commit 18e66b695e78 ("x86/shstk: Add Kconfig option for shadow
> stack"), AS_WRUSS is set even in 32-bit .configs. It is due to how
> Kbuild works. .config is not considered during make oldconfig (and other
> make *config), so standard (64-bit) gcc is invoked from 'as-instr'
> Kbuild tests. And such gcc indeed reports that wruss is supported, so
> AS_WRUSS=y is set.
>
> Provided the wruss instruction is 64-bit only (and used in pure 64-bit
> X86_USER_SHADOW_STACK), it has little sense to have AS_WRUSS=y set on
> 32-bit.
>
> Therefore, make the whole test dependent on X86_64 to ensure it's set
> only on 64-bit.
>
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Borislav Petkov (AMD) <bp@alien8.de>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Mike Rapoport (IBM) <rppt@kernel.org>
> Cc: Pengfei Xu <pengfei.xu@intel.com>
> Cc: John Allen <john.allen@amd.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> ---
> arch/x86/Kconfig.assembler | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/Kconfig.assembler b/arch/x86/Kconfig.assembler
> index 8ad41da301e5..a5b5241711e3 100644
> --- a/arch/x86/Kconfig.assembler
> +++ b/arch/x86/Kconfig.assembler
> @@ -27,5 +27,6 @@ config AS_GFNI
>
> config AS_WRUSS
> def_bool $(as-instr,wrussq %rax$(comma)(%rbx))
> + depends on X86_64
> help
> Supported by binutils >= 2.31 and LLVM integrated assembler
> --
> 2.42.0
>
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Oct 31, 2023 at 8:26 PM Borislav Petkov <bp@alien8.de> wrote:
>
> + linux-kbuild@vger.kernel.org
>
> On Tue, Oct 31, 2023 at 11:21:11AM +0100, Jiri Slaby (SUSE) wrote:
> > Since commit 18e66b695e78 ("x86/shstk: Add Kconfig option for shadow
> > stack"), AS_WRUSS is set even in 32-bit .configs. It is due to how
> > Kbuild works. .config is not considered during make oldconfig (and other
> > make *config), so standard (64-bit) gcc is invoked from 'as-instr'
> > Kbuild tests.
I do not mind either way.
Please note "depends on X86_64" cannot prevent gcc
from running here.
$(as-instr,wrussq %rax$(comma)(%rbx)) is replaced with 'y'
while parsing the Kconfig files.
I want to change it in the future, but that is how Kconfig works now.
You don't save the cost of running the compiler.
> And such gcc indeed reports that wruss is supported, so
> > AS_WRUSS=y is set.
> >
> > Provided the wruss instruction is 64-bit only (and used in pure 64-bit
> > X86_USER_SHADOW_STACK), it has little sense to have AS_WRUSS=y set on
> > 32-bit.
> >
> > Therefore, make the whole test dependent on X86_64 to ensure it's set
> > only on 64-bit.
> >
> > Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> > Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Cc: Borislav Petkov (AMD) <bp@alien8.de>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Mike Rapoport (IBM) <rppt@kernel.org>
> > Cc: Pengfei Xu <pengfei.xu@intel.com>
> > Cc: John Allen <john.allen@amd.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: x86@kernel.org
> > ---
> > arch/x86/Kconfig.assembler | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/x86/Kconfig.assembler b/arch/x86/Kconfig.assembler
> > index 8ad41da301e5..a5b5241711e3 100644
> > --- a/arch/x86/Kconfig.assembler
> > +++ b/arch/x86/Kconfig.assembler
> > @@ -27,5 +27,6 @@ config AS_GFNI
> >
> > config AS_WRUSS
> > def_bool $(as-instr,wrussq %rax$(comma)(%rbx))
> > + depends on X86_64
> > help
> > Supported by binutils >= 2.31 and LLVM integrated assembler
> > --
> > 2.42.0
> >
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
--
Best Regards
Masahiro Yamada
On Tue, Oct 31, 2023 at 09:20:33PM +0900, Masahiro Yamada wrote:
> Please note "depends on X86_64" cannot prevent gcc
> from running here.
>
> $(as-instr,wrussq %rax$(comma)(%rbx)) is replaced with 'y'
> while parsing the Kconfig files.
>
Then what is this fixing?
Jiri, what openSUSE kernel build issues were you referring to on IRC?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 31. 10. 23, 14:34, Borislav Petkov wrote: > On Tue, Oct 31, 2023 at 09:20:33PM +0900, Masahiro Yamada wrote: >> Please note "depends on X86_64" cannot prevent gcc >> from running here. >> >> $(as-instr,wrussq %rax$(comma)(%rbx)) is replaced with 'y' >> while parsing the Kconfig files. >> > Then what is this fixing? > > Jiri, what openSUSE kernel build issues were you referring to on IRC? As I wrote, we have a config which maintainers feed through oldconfig when updating the kernel. This time we got WRUSS=y. It was stored in git and later passed to build the kernel. And we received a different config from the build (WRUSS=n). We diff the two and error out if the built kernel differs from the passed configuration. Which is the case here. hth, -- js
On Tue, Oct 31, 2023 at 02:59:39PM +0100, Jiri Slaby wrote:
> As I wrote, we have a config which maintainers feed through oldconfig when
> updating the kernel. This time we got WRUSS=y. It was stored in git and
> later passed to build the kernel. And we received a different config from
> the build (WRUSS=n).
>
> We diff the two and error out if the built kernel differs from the passed
> configuration. Which is the case here.
I sure hope that diffing script can ignore Kconfig items. Because this
doesn't sound like a reason to "fix" the upstream kernel and as Masahiro
says, it doesn't change anything anyway.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 31. 10. 23, 15:05, Borislav Petkov wrote: > On Tue, Oct 31, 2023 at 02:59:39PM +0100, Jiri Slaby wrote: >> As I wrote, we have a config which maintainers feed through oldconfig when >> updating the kernel. This time we got WRUSS=y. It was stored in git and >> later passed to build the kernel. And we received a different config from >> the build (WRUSS=n). >> >> We diff the two and error out if the built kernel differs from the passed >> configuration. Which is the case here. > > I sure hope that diffing script can ignore Kconfig items. Definitely, I can add an ignore on that level. > Because this > doesn't sound like a reason to "fix" the upstream kernel and as Masahiro > says, it doesn't change anything anyway. It changes what's in .config and corresponds 1:1 to what is used during the build. Now, I have a .config from "make oldconfig". But "make all" will read it and flips the value. (That is IMHO misleading, but I can live with that, provided I now know. Not sure about others though.) thanks, -- js suse labs
© 2016 - 2025 Red Hat, Inc.