Re: [PATCH 2/3] powerpc: Fix undefined static key

Le 17/08/2021 à 04:44, Joel Stanley a écrit :

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(+)

diff --git a/arch/powerpc/include/asm/security_features.h b/arch/powerpc/include/asm/security_features.h
index 792eefaf230b..46ade7927a4c 100644
--- a/arch/powerpc/include/asm/security_features.h
+++ b/arch/powerpc/include/asm/security_features.h

@@ -39,6 +39,9 @@ static inline bool security_ftr_enabled(u64 feature)
       return !!(powerpc_security_features & feature);
   }

+#ifndef CONFIG_PPC_BARRIER_NOSPEC
+DEFINE_STATIC_KEY_FALSE(uaccess_flush_key);
+#endif

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?