[PATCH 0/3] powerpc/64s: Fix PPC_BARRIER_NOSPEC=n

[PATCH 0/3] powerpc/64s: Fix PPC_BARRIER_NOSPEC=n

[Joel Stanley]

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

[PATCH 1/3] powerpc/64s: Fix build when !PPC_BARRIER_NOSPEC

[Joel Stanley]

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

diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
index 6c1a7d217d1a..71012284c044 100644
--- a/arch/powerpc/include/asm/setup.h
+++ b/arch/powerpc/include/asm/setup.h
@@ -66,8 +66,6 @@ extern bool barrier_nospec_enabled;
 
 #ifdef CONFIG_PPC_BARRIER_NOSPEC
 void do_barrier_nospec_fixups_range(bool enable, void *start, void *end);
-#else
-static inline void do_barrier_nospec_fixups_range(bool enable, void *start, void *end) { }
 #endif
 
 #ifdef CONFIG_PPC_FSL_BOOK3E
-- 
2.32.0

Re: [PATCH 1/3] powerpc/64s: Fix build when !PPC_BARRIER_NOSPEC

[Christophe Leroy]

Le 16/08/2021 à 10:24, Joel Stanley a écrit :
> 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.

> 
> 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(-)
> 
> diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
> index 6c1a7d217d1a..71012284c044 100644
> --- a/arch/powerpc/include/asm/setup.h
> +++ b/arch/powerpc/include/asm/setup.h
> @@ -66,8 +66,6 @@ extern bool barrier_nospec_enabled;
>   
>   #ifdef CONFIG_PPC_BARRIER_NOSPEC
>   void do_barrier_nospec_fixups_range(bool enable, void *start, void *end);
> -#else
> -static inline void do_barrier_nospec_fixups_range(bool enable, void *start, void *end) { }
>   #endif
>   
>   #ifdef CONFIG_PPC_FSL_BOOK3E
> 

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

[Joel Stanley]

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
 
 // Features indicating support for Spectre/Meltdown mitigations
 
-- 
2.32.0

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

[Christophe Leroy]

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)



>   
>   // Features indicating support for Spectre/Meltdown mitigations
>   
> 

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

[Joel Stanley]

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)

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?

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

[Christophe Leroy]

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?
> 

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

[Michael Ellerman]

Joel Stanley <joel@jms.id.au> 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

[PATCH 3/3] powerpc/microwatt: CPU doesn't (yet) have speculation bugs

[Joel Stanley]

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 arch/powerpc/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 663766fbf505..d5af6667c206 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -279,6 +279,7 @@ config PPC_BARRIER_NOSPEC
 	bool
 	default y
 	depends on PPC_BOOK3S_64 || PPC_FSL_BOOK3E
+	depends on !PPC_MICROWATT
 
 config EARLY_PRINTK
 	bool
-- 
2.32.0

Re: [PATCH 3/3] powerpc/microwatt: CPU doesn't (yet) have speculation bugs

[Christophe Leroy]

Le 16/08/2021 à 10:24, Joel Stanley a écrit :
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>   arch/powerpc/Kconfig | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 663766fbf505..d5af6667c206 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -279,6 +279,7 @@ config PPC_BARRIER_NOSPEC
>   	bool
>   	default y
>   	depends on PPC_BOOK3S_64 || PPC_FSL_BOOK3E
> +	depends on !PPC_MICROWATT

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

>   
>   config EARLY_PRINTK
>   	bool
>