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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 17/08/2021 à 04:44, Joel Stanley a écrit :
> On Mon, 16 Aug 2021 at 08:39, Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>>
>>
>>
>> Le 16/08/2021 à 10:24, Joel Stanley a écrit :
>>> 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?
>