From: Joel Stanley <joel@jms.id.au> Date: 2021-08-16 08:25:08
When disabling PPC_BARRIER_NOSPEC on Microwatt to see if it improved
boot time, I discovered the build was broken (first patch). This got
worse between when I first tried and now (second patch).
The third patch disables PPC_BARRIER_NOSPEC when building for Microwatt.
This one is optional, as it doesn't seem to change boot speed with the
current Microwatt design on an Arty.
Joel Stanley (3):
powerpc/64s: Fix build when !PPC_BARRIER_NOSPEC
powerpc: Fix undefined static key
powerpc/microwatt: CPU doesn't (yet) have speculation bugs
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/security_features.h | 3 +++
arch/powerpc/include/asm/setup.h | 2 --
3 files changed, 4 insertions(+), 2 deletions(-)
--
2.32.0
From: Joel Stanley <joel@jms.id.au> Date: 2021-08-16 08:25:46
When disabling PPC_BARRIER_NOSPEC the do_barrier_nospec_fixups_range
definition is still included, as well as a stub in asm/setup.h:
../arch/powerpc/lib/feature-fixups.c:502:6: error: redefinition of ‘do_barrier_nospec_fixu>
502 | void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, void *fixup_en>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../arch/powerpc/lib/feature-fixups.c:23:
../arch/powerpc/include/asm/setup.h:70:20: note: previous definition of ‘do_barrier_nospec>
70 | static inline void do_barrier_nospec_fixups_range(bool enable, void *start, void *>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I assume the intent was to put the just do_barrier_nospec_fixups
behind PPC_BARRIER_NOSPEC and let the compiler drop _range when there
are no users. (There is a caller in module.c, but this is behind
PPC_BARRIER_NOSPEC).
This makes PPC_BOOK3S_64 match how the PPC_FSL_BOOK3E build works.
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
arch/powerpc/include/asm/setup.h | 2 --
1 file changed, 2 deletions(-)
From: Joel Stanley <joel@jms.id.au> Date: 2021-08-16 08:26:23
When CONFIG_PPC_BARRIER_NOSPEC=n, security.c is not built leading to a
missing definition of uaccess_flush_key.
LD vmlinux.o
MODPOST vmlinux.symvers
MODINFO modules.builtin.modinfo
GEN modules.builtin
LD .tmp_vmlinux.kallsyms1
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/align.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/signal_64.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/process.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/traps.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/hw_breakpoint_constraints.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/ptrace/ptrace.o:(.toc+0x0): more undefined references to `uaccess_flush_key' follow
make[1]: *** [Makefile:1176: vmlinux] Error 1
Hack one in to fix the build.
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
arch/powerpc/include/asm/security_features.h | 3 +++
1 file changed, 3 insertions(+)
When disabling PPC_BARRIER_NOSPEC the do_barrier_nospec_fixups_range
definition is still included, as well as a stub in asm/setup.h:
../arch/powerpc/lib/feature-fixups.c:502:6: error: redefinition of ‘do_barrier_nospec_fixu>
502 | void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, void *fixup_en>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../arch/powerpc/lib/feature-fixups.c:23:
../arch/powerpc/include/asm/setup.h:70:20: note: previous definition of ‘do_barrier_nospec>
70 | static inline void do_barrier_nospec_fixups_range(bool enable, void *start, void *>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I assume the intent was to put the just do_barrier_nospec_fixups
behind PPC_BARRIER_NOSPEC and let the compiler drop _range when there
are no users. (There is a caller in module.c, but this is behind
PPC_BARRIER_NOSPEC).
The compiler won't drop do_barrier_nospec_fixups_range() because it is not static.
quoted hunk
This makes PPC_BOOK3S_64 match how the PPC_FSL_BOOK3E build works.
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
arch/powerpc/include/asm/setup.h | 2 --
1 file changed, 2 deletions(-)
When CONFIG_PPC_BARRIER_NOSPEC=n, security.c is not built leading to a
missing definition of uaccess_flush_key.
LD vmlinux.o
MODPOST vmlinux.symvers
MODINFO modules.builtin.modinfo
GEN modules.builtin
LD .tmp_vmlinux.kallsyms1
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/align.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/signal_64.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/process.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/traps.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/hw_breakpoint_constraints.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/ptrace/ptrace.o:(.toc+0x0): more undefined references to `uaccess_flush_key' follow
make[1]: *** [Makefile:1176: vmlinux] Error 1
Hack one in to fix the build.
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
arch/powerpc/include/asm/security_features.h | 3 +++
1 file changed, 3 insertions(+)
It will then be re-defined by each file that includes asm/security_features.h ....
You can't use a DEFINE_ in a .h
You have to fix the problem at its source.
Cleanest way I see it to modify asm/book3s/64/kup.h and something like
if (IS_ENABLED(CONFIG_PPC_BARRIER_NOSPEC) && static_branch_unlikely(&uaccess_flush_key)
// Features indicating support for Spectre/Meltdown mitigations
Not sure it is a good idea to disable it completely when PPC_MICROWATT is selected. Don't we want to
be able to build generic kernels that can run on any book3s/64 ?
Maybe you should change the default instead, something like:
bool
default y if !PPC_MICROWATT
depends on PPC_BOOK3S_64 || PPC_FSL_BOOK3E
From: Joel Stanley <joel@jms.id.au> Date: 2021-08-17 02:44:58
On Mon, 16 Aug 2021 at 08:39, Christophe Leroy
[off-list ref] wrote:
Le 16/08/2021 à 10:24, Joel Stanley a écrit :
quoted
When CONFIG_PPC_BARRIER_NOSPEC=n, security.c is not built leading to a
missing definition of uaccess_flush_key.
LD vmlinux.o
MODPOST vmlinux.symvers
MODINFO modules.builtin.modinfo
GEN modules.builtin
LD .tmp_vmlinux.kallsyms1
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/align.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/signal_64.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/process.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/traps.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/hw_breakpoint_constraints.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/ptrace/ptrace.o:(.toc+0x0): more undefined references to `uaccess_flush_key' follow
make[1]: *** [Makefile:1176: vmlinux] Error 1
Hack one in to fix the build.
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
arch/powerpc/include/asm/security_features.h | 3 +++
1 file changed, 3 insertions(+)
It will then be re-defined by each file that includes asm/security_features.h ....
You can't use a DEFINE_ in a .h
You have to fix the problem at its source.
Cleanest way I see it to modify asm/book3s/64/kup.h and something like
if (IS_ENABLED(CONFIG_PPC_BARRIER_NOSPEC) && static_branch_unlikely(&uaccess_flush_key)
This won't work either as there's no declaration for uaccess_flush_key:
arch/powerpc/include/asm/book3s/64/kup.h:411:78: error:
‘uaccess_flush_key’ undeclared (first use in this function)
We could add an extern for it, but that is distasteful as the static
key API suggests using the structs directly is deprecated, and the
macros we're supposed to use perform initialisation.
Could we assume microwatt-like platforms will not gain pkeys support,
and have a stubbed out set of prevent/restore_user_access for systems
that turn off either pkeys or BARRIER_NOSPEC?
Or do we get rid of PPC_BARRIER_NOSPEC as an option, and have machines
rely on disabling it at runtime?
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-08-17 02:53:21
Joel Stanley [off-list ref] writes:
When CONFIG_PPC_BARRIER_NOSPEC=n, security.c is not built leading to a
missing definition of uaccess_flush_key.
LD vmlinux.o
MODPOST vmlinux.symvers
MODINFO modules.builtin.modinfo
GEN modules.builtin
LD .tmp_vmlinux.kallsyms1
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/align.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/signal_64.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/process.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/traps.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/hw_breakpoint_constraints.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/ptrace/ptrace.o:(.toc+0x0): more undefined references to `uaccess_flush_key' follow
make[1]: *** [Makefile:1176: vmlinux] Error 1
Hack one in to fix the build.
Yeah sorry that is a bit of a hack :)
I think the root cause here is that we don't have a CONFIG for "want
security workaround stuff", because so far we haven't had a CPU that
wants to turn that all off.
The generic code uses CONFIG_GENERIC_CPU_VULNERABILITIES, so I guess we
should follow that example and add PPC_CPU_VULNERABILITIES.
Then we'd use that to disable all the security.c stuff, and
PPC_BARRIER_NOSPEC would depend on it also.
Then we could allow configuring that off for Microwatt, or possibly for
all platforms if people want to do that.
cheers
On Mon, 16 Aug 2021 at 08:39, Christophe Leroy
[off-list ref] wrote:
quoted
Le 16/08/2021 à 10:24, Joel Stanley a écrit :
quoted
When CONFIG_PPC_BARRIER_NOSPEC=n, security.c is not built leading to a
missing definition of uaccess_flush_key.
LD vmlinux.o
MODPOST vmlinux.symvers
MODINFO modules.builtin.modinfo
GEN modules.builtin
LD .tmp_vmlinux.kallsyms1
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/align.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/signal_64.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/process.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/traps.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/hw_breakpoint_constraints.o:(.toc+0x0): undefined reference to `uaccess_flush_key'
powerpc64le-linux-gnu-ld: arch/powerpc/kernel/ptrace/ptrace.o:(.toc+0x0): more undefined references to `uaccess_flush_key' follow
make[1]: *** [Makefile:1176: vmlinux] Error 1
Hack one in to fix the build.
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
arch/powerpc/include/asm/security_features.h | 3 +++
1 file changed, 3 insertions(+)
It will then be re-defined by each file that includes asm/security_features.h ....
You can't use a DEFINE_ in a .h
You have to fix the problem at its source.
Cleanest way I see it to modify asm/book3s/64/kup.h and something like
if (IS_ENABLED(CONFIG_PPC_BARRIER_NOSPEC) && static_branch_unlikely(&uaccess_flush_key)
This won't work either as there's no declaration for uaccess_flush_key:
arch/powerpc/include/asm/book3s/64/kup.h:411:78: error:
‘uaccess_flush_key’ undeclared (first use in this function)
I can't understand that.
According to your commit message, you have reached linking step. It means that the necessary
declarations were present.
Adding a condition on IS_ENABLED(CONFIG_PPC_BARRIER_NOSPEC) will get you rid of the code past that
condition at compile step, then linking will succeed because the code referencing
'uaccess_flush_key' won't be there anymore when you proceed with linking.
We could add an extern for it, but that is distasteful as the static
key API suggests using the structs directly is deprecated, and the
macros we're supposed to use perform initialisation.
You already have DECLARE_STATIC_KEY_FALSE(uaccess_flush_key); in <asm/book3s/64/kup.h>
That doesn't initialise anythink but provides the declaration.
Could we assume microwatt-like platforms will not gain pkeys support,
and have a stubbed out set of prevent/restore_user_access for systems
that turn off either pkeys or BARRIER_NOSPEC?
Or do we get rid of PPC_BARRIER_NOSPEC as an option, and have machines
rely on disabling it at runtime?