[v2,6/6] powerpc/64: Add tests for out-of-line static calls

KUnit tests for the various combinations of caller/trampoline/target and
kernel/module. They must be run from a module loaded at runtime to
guarantee they have a different TOC to the kernel.

The tests try to mitigate the chance of panicing by restoring the
TOC after every static call. Not all possible errors can be caught
by this (we can't stop a trampoline from using a bad TOC itself),
but it makes certain errors easier to debug.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 arch/powerpc/Kconfig                   |  10 +
 arch/powerpc/kernel/Makefile           |   1 +
 arch/powerpc/kernel/static_call.c      |  61 ++++++
 arch/powerpc/kernel/static_call_test.c | 251 +++++++++++++++++++++++++
 arch/powerpc/kernel/static_call_test.h |  56 ++++++
 5 files changed, 379 insertions(+)
 create mode 100644 arch/powerpc/kernel/static_call_test.c
 create mode 100644 arch/powerpc/kernel/static_call_test.h

Le 26/09/2022 à 08:43, Benjamin Gray a écrit :
> KUnit tests for the various combinations of caller/trampoline/target and
> kernel/module. They must be run from a module loaded at runtime to
> guarantee they have a different TOC to the kernel.
> 
> The tests try to mitigate the chance of panicing by restoring the
> TOC after every static call. Not all possible errors can be caught
> by this (we can't stop a trampoline from using a bad TOC itself),
> but it makes certain errors easier to debug.
> 
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
>   arch/powerpc/Kconfig                   |  10 +
>   arch/powerpc/kernel/Makefile           |   1 +
>   arch/powerpc/kernel/static_call.c      |  61 ++++++
>   arch/powerpc/kernel/static_call_test.c | 251 +++++++++++++++++++++++++
>   arch/powerpc/kernel/static_call_test.h |  56 ++++++
>   5 files changed, 379 insertions(+)
>   create mode 100644 arch/powerpc/kernel/static_call_test.c
>   create mode 100644 arch/powerpc/kernel/static_call_test.h
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index e7a66635eade..0ca60514c0e2 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -1023,6 +1023,16 @@ config PPC_RTAS_FILTER
>   	  Say Y unless you know what you are doing and the filter is causing
>   	  problems for you.
>   
> +config PPC_STATIC_CALL_KUNIT_TEST
> +	tristate "KUnit tests for PPC64 ELF ABI V2 static calls"
> +	default KUNIT_ALL_TESTS
> +	depends on HAVE_STATIC_CALL && PPC64_ELF_ABI_V2 && KUNIT && m

Is there a reason why it is dedicated to PPC64 ? In that case, can you 
make it explicit with the name of the config option, and with the name 
of the file below ?

> +	help
> +	  Tests that check the TOC is kept consistent across all combinations
> +	  of caller/trampoline/target being kernel/module. Must be built as a
> +	  module and loaded at runtime to ensure the module has a different
> +	  TOC to the kernel.
> +
>   endmenu
>   
>   config ISA_DMA_API
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index a30d0d0f5499..22c07e3d34df 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -131,6 +131,7 @@ obj-$(CONFIG_RELOCATABLE)	+= reloc_$(BITS).o
>   obj-$(CONFIG_PPC32)		+= entry_32.o setup_32.o early_32.o
>   obj-$(CONFIG_PPC64)		+= dma-iommu.o iommu.o
>   obj-$(CONFIG_HAVE_STATIC_CALL)	+= static_call.o
> +obj-$(CONFIG_PPC_STATIC_CALL_KUNIT_TEST)	+= static_call_test.o
>   obj-$(CONFIG_KGDB)		+= kgdb.o
>   obj-$(CONFIG_BOOTX_TEXT)	+= btext.o
>   obj-$(CONFIG_SMP)		+= smp.o
> diff --git a/arch/powerpc/kernel/static_call.c b/arch/powerpc/kernel/static_call.c
> index ecbb74e1b4d3..8d338917b70e 100644
> --- a/arch/powerpc/kernel/static_call.c
> +++ b/arch/powerpc/kernel/static_call.c
> @@ -113,3 +113,64 @@ void arch_static_call_transform(void *site, void *tramp, void *func, bool tail)
>   		panic("%s: patching failed %pS at %pS\n", __func__, func, tramp);
>   }
>   EXPORT_SYMBOL_GPL(arch_static_call_transform);
> +
> +
> +#if IS_MODULE(CONFIG_PPC_STATIC_CALL_KUNIT_TEST)
> +
> +#include "static_call_test.h"
> +
> +int ppc_sc_kernel_target_1(struct kunit* test)
> +{
> +	toc_fixup(test);
> +	return 1;
> +}
> +
> +int ppc_sc_kernel_target_2(struct kunit* test)
> +{
> +	toc_fixup(test);
> +	return 2;
> +}
> +
> +DEFINE_STATIC_CALL(ppc_sc_kernel, ppc_sc_kernel_target_1);
> +
> +int ppc_sc_kernel_call(struct kunit* test)
> +{
> +	return PROTECTED_SC(test, int, static_call(ppc_sc_kernel)(test));
> +}
> +
> +int ppc_sc_kernel_call_indirect(struct kunit* test, int (*fn)(struct kunit*))
> +{
> +	return PROTECTED_SC(test, int, fn(test));
> +}
> +
> +long ppc_sc_kernel_target_big(struct kunit* test,
> +			      long a,
> +			      long b,
> +			      long c,
> +			      long d,
> +			      long e,
> +			      long f,
> +			      long g,
> +			      long h,
> +			      long i)
> +{
> +	toc_fixup(test);
> +	KUNIT_EXPECT_EQ(test, a, b);
> +	KUNIT_EXPECT_EQ(test, a, c);
> +	KUNIT_EXPECT_EQ(test, a, d);
> +	KUNIT_EXPECT_EQ(test, a, e);
> +	KUNIT_EXPECT_EQ(test, a, f);
> +	KUNIT_EXPECT_EQ(test, a, g);
> +	KUNIT_EXPECT_EQ(test, a, h);
> +	KUNIT_EXPECT_EQ(test, a, i);
> +	return ~a;
> +}
> +
> +EXPORT_SYMBOL_GPL(ppc_sc_kernel_target_1);
> +EXPORT_SYMBOL_GPL(ppc_sc_kernel_target_2);
> +EXPORT_SYMBOL_GPL(ppc_sc_kernel_target_big);
> +EXPORT_STATIC_CALL_GPL(ppc_sc_kernel);
> +EXPORT_SYMBOL_GPL(ppc_sc_kernel_call);
> +EXPORT_SYMBOL_GPL(ppc_sc_kernel_call_indirect);
> +
> +#endif /* IS_MODULE(CONFIG_PPC_STATIC_CALL_KUNIT_TEST) */
> diff --git a/arch/powerpc/kernel/static_call_test.c b/arch/powerpc/kernel/static_call_test.c
> new file mode 100644
> index 000000000000..2d69524d935f
> --- /dev/null
> +++ b/arch/powerpc/kernel/static_call_test.c
> @@ -0,0 +1,251 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "static_call_test.h"
> +
> +#include <linux/kconfig.h>
> +#include <linux/module.h>
> +#include <linux/static_call.h>
> +
> +/*
> + * Tests to ensure correctness in a variety of cases for static calls.
> + *
> + * The tests focus on ensuring the TOC is kept consistent across the
> + * module-kernel boundary, as compilers can't see that a trampoline
> + * defined locally to a caller might be jumping to a function with a
> + * different TOC. So it's important that these tests are compiled as
> + * a module to ensure the TOC will be different to the kernel's.
> + */
> +
> +/* Utils to hold a copy of the old register values while we test.
> + *
> + * We can't use the KUnit init/exit hooks because when the hooks and
> + * test cases return they will be in the KUnit context that doesn't know
> + * we've reserved and modified some non-volatile registers.
> + */
> +static void* regsaves[3];
> +
> +#define SAVE_REGS() \
> +	regsaves[0] = actual_toc; \
> +	regsaves[1] = module_toc; \
> +	regsaves[2] = kernel_toc; \
> +	module_toc = current_toc; \
> +	kernel_toc = (void*) get_paca()->kernel_toc;
> +
> +#define RESTORE_REGS() \
> +	actual_toc = regsaves[0]; \
> +	module_toc = regsaves[1]; \
> +	kernel_toc = regsaves[2];
> +
> +static int module_target_11(struct kunit *test)
> +{
> +	toc_fixup(test);
> +	return 11;
> +}
> +
> +static int module_target_12(struct kunit *test)
> +{
> +	toc_fixup(test);
> +	return 12;
> +}
> +
> +DEFINE_STATIC_CALL(module_sc, module_target_11);
> +
> +DEFINE_STATIC_CALL_RET0(module_sc_ret0, int(void));
> +DEFINE_STATIC_CALL_NULL(module_sc_null, int(int));
> +
> +static int add_one(int *val)
> +{
> +	return (*val)++;
> +}
> +
> +static void null_function_test(struct kunit *test)
> +{
> +	int val = 0;
> +
> +	SAVE_REGS();
> +
> +	/* Check argument unconditionally evaluated */
> +	static_call_cond(module_sc_null)(add_one(&val));
> +	KUNIT_ASSERT_EQ(test, 1, val);
> +
> +	RESTORE_REGS();
> +}
> +
> +static void return_zero_test(struct kunit *test)
> +{
> +	int ret;
> +
> +	SAVE_REGS();
> +
> +	ret = PROTECTED_SC(test, int, static_call(module_sc_ret0)());
> +	KUNIT_ASSERT_EQ(test, 0, ret);
> +
> +	static_call_update(ppc_sc_kernel, (void*)__static_call_return0);
> +	ret = PROTECTED_SC(test, int, static_call(ppc_sc_kernel)(test));
> +	KUNIT_ASSERT_EQ(test, 0, ret);
> +
> +	static_call_update(module_sc, (void*)__static_call_return0);
> +	ret = PROTECTED_SC(test, int, static_call(module_sc)(test));
> +	KUNIT_ASSERT_EQ(test, 0, ret);
> +
> +	RESTORE_REGS();
> +}
> +
> +static void kernel_kernel_kernel_test(struct kunit *test)
> +{
> +	SAVE_REGS();
> +
> +	static_call_update(ppc_sc_kernel, ppc_sc_kernel_target_1);
> +	KUNIT_ASSERT_EQ(test, 1, ppc_sc_kernel_call(test));
> +
> +	static_call_update(ppc_sc_kernel, ppc_sc_kernel_target_2);
> +	KUNIT_ASSERT_EQ(test, 2, ppc_sc_kernel_call(test));
> +
> +	RESTORE_REGS();
> +}
> +
> +static void kernel_kernel_module_test(struct kunit *test)
> +{
> +	SAVE_REGS();
> +
> +	static_call_update(ppc_sc_kernel, module_target_11);
> +	KUNIT_ASSERT_EQ(test, 11, ppc_sc_kernel_call(test));
> +
> +	static_call_update(ppc_sc_kernel, module_target_12);
> +	KUNIT_ASSERT_EQ(test, 12, ppc_sc_kernel_call(test));
> +
> +	RESTORE_REGS();
> +}
> +
> +static void kernel_module_kernel_test(struct kunit *test)
> +{
> +	SAVE_REGS();
> +
> +	static_call_update(module_sc, ppc_sc_kernel_target_1);
> +	KUNIT_ASSERT_EQ(test, 1, ppc_sc_kernel_call_indirect(test, static_call(module_sc)));
> +
> +	static_call_update(module_sc, ppc_sc_kernel_target_2);
> +	KUNIT_ASSERT_EQ(test, 2, ppc_sc_kernel_call_indirect(test, static_call(module_sc)));
> +
> +	RESTORE_REGS();
> +}
> +
> +static void kernel_module_module_test(struct kunit *test)
> +{
> +	SAVE_REGS();
> +
> +	static_call_update(module_sc, module_target_11);
> +	KUNIT_ASSERT_EQ(test, 11, ppc_sc_kernel_call_indirect(test, static_call(module_sc)));
> +
> +	static_call_update(module_sc, module_target_12);
> +	KUNIT_ASSERT_EQ(test, 12, ppc_sc_kernel_call_indirect(test, static_call(module_sc)));
> +
> +	RESTORE_REGS();
> +}
> +
> +static void module_kernel_kernel_test(struct kunit *test)
> +{
> +	int ret;
> +
> +	SAVE_REGS();
> +
> +	static_call_update(ppc_sc_kernel, ppc_sc_kernel_target_1);
> +	ret = PROTECTED_SC(test, int, static_call(ppc_sc_kernel)(test));
> +	KUNIT_ASSERT_EQ(test, 1, ret);
> +
> +	static_call_update(ppc_sc_kernel, ppc_sc_kernel_target_2);
> +	ret = PROTECTED_SC(test, int, static_call(ppc_sc_kernel)(test));
> +	KUNIT_ASSERT_EQ(test, 2, ret);
> +
> +	RESTORE_REGS();
> +}
> +
> +static void module_kernel_module_test(struct kunit *test)
> +{
> +	int ret;
> +
> +	SAVE_REGS();
> +
> +	static_call_update(ppc_sc_kernel, module_target_11);
> +	ret = PROTECTED_SC(test, int, static_call(ppc_sc_kernel)(test));
> +	KUNIT_ASSERT_EQ(test, 11, ret);
> +
> +	static_call_update(ppc_sc_kernel, module_target_12);
> +	ret = PROTECTED_SC(test, int, static_call(ppc_sc_kernel)(test));
> +	KUNIT_ASSERT_EQ(test, 12, ret);
> +
> +	RESTORE_REGS();
> +}
> +
> +static void module_module_kernel_test(struct kunit *test)
> +{
> +	int ret;
> +
> +	SAVE_REGS();
> +
> +	static_call_update(module_sc, ppc_sc_kernel_target_1);
> +	ret = PROTECTED_SC(test, int, static_call(module_sc)(test));
> +	KUNIT_ASSERT_EQ(test, 1, ret);
> +
> +	static_call_update(module_sc, ppc_sc_kernel_target_2);
> +	ret = PROTECTED_SC(test, int, static_call(module_sc)(test));
> +	KUNIT_ASSERT_EQ(test, 2, ret);
> +
> +	RESTORE_REGS();
> +}
> +
> +static void module_module_module_test(struct kunit *test)
> +{
> +	int ret;
> +
> +	SAVE_REGS();
> +
> +	static_call_update(module_sc, module_target_11);
> +	ret = PROTECTED_SC(test, int, static_call(module_sc)(test));
> +	KUNIT_ASSERT_EQ(test, 11, ret);
> +
> +	static_call_update(module_sc, module_target_12);
> +	ret = PROTECTED_SC(test, int, static_call(module_sc)(test));
> +	KUNIT_ASSERT_EQ(test, 12, ret);
> +
> +	RESTORE_REGS();
> +}
> +
> +DEFINE_STATIC_CALL(module_sc_stack_params, ppc_sc_kernel_target_big);
> +
> +static void stack_parameters_test(struct kunit *test)
> +{
> +	long m = 0x1234567887654321;
> +	long ret;
> +
> +	SAVE_REGS();
> +
> +	ret = PROTECTED_SC(test, long, static_call(module_sc_stack_params)(test, m, m, m, m, m, m, m, m, m));
> +	KUNIT_ASSERT_EQ(test, ~m, ret);
> +
> +	RESTORE_REGS();
> +}
> +
> +static struct kunit_case static_call_test_cases[] = {
> +	KUNIT_CASE(null_function_test),
> +	KUNIT_CASE(return_zero_test),
> +	KUNIT_CASE(stack_parameters_test),
> +	KUNIT_CASE(kernel_kernel_kernel_test),
> +	KUNIT_CASE(kernel_kernel_module_test),
> +	KUNIT_CASE(kernel_module_kernel_test),
> +	KUNIT_CASE(kernel_module_module_test),
> +	KUNIT_CASE(module_kernel_kernel_test),
> +	KUNIT_CASE(module_kernel_module_test),
> +	KUNIT_CASE(module_module_kernel_test),
> +	KUNIT_CASE(module_module_module_test),
> +	{}
> +};
> +
> +static struct kunit_suite ppc_static_call_test_suite = {
> +	.name = "ppc-static-call",
> +	.test_cases = static_call_test_cases,
> +};
> +kunit_test_suite(ppc_static_call_test_suite);
> +
> +MODULE_AUTHOR("Benjamin Gray <bgray@linux.ibm.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/arch/powerpc/kernel/static_call_test.h b/arch/powerpc/kernel/static_call_test.h
> new file mode 100644
> index 000000000000..597da89297fa
> --- /dev/null
> +++ b/arch/powerpc/kernel/static_call_test.h
> @@ -0,0 +1,56 @@
> +#ifndef _POWERPC_STATIC_CALL_TEST_
> +#define _POWERPC_STATIC_CALL_TEST_
> +
> +#include <kunit/test.h>
> +
> +/* Reserve these registers for testing so that a TOC error
> + * doesn't necessarily crash the whole kernel.
> + *
> + * The tests ensure the contents are restored before returning.
> + */
> +register void* current_toc asm ("r2");
> +register void* actual_toc asm ("r14");  /* Copy of TOC before fixup */
> +register void* module_toc asm ("r15");
> +register void* kernel_toc asm ("r16");
> +
> +DECLARE_STATIC_CALL(ppc_sc_kernel, int(struct kunit*));
> +int ppc_sc_kernel_target_1(struct kunit* test);
> +int ppc_sc_kernel_target_2(struct kunit* test);
> +long ppc_sc_kernel_target_big(struct kunit* test,
> +				     long a,
> +				     long b,
> +				     long c,
> +				     long d,
> +				     long e,
> +				     long f,
> +				     long g,
> +				     long h,
> +				     long i);
> +int ppc_sc_kernel_call(struct kunit* test);
> +int ppc_sc_kernel_call_indirect(struct kunit* test, int(*fn)(struct kunit*));
> +
> +#ifdef MODULE
> +
> +#define toc_fixup(test) \
> +	actual_toc = current_toc; \
> +	current_toc = module_toc; \
> +	KUNIT_EXPECT_PTR_EQ(test, module_toc, actual_toc)
> +
> +#else /* KERNEL */
> +
> +#define toc_fixup(test) \
> +	actual_toc = current_toc; \
> +	current_toc = kernel_toc; \
> +	KUNIT_EXPECT_PTR_EQ(test, kernel_toc, actual_toc)
> +
> +#endif /* MODULE */
> +
> +#define PROTECTED_SC(test, ret_type, call) \
> +({ \
> +	ret_type ret; \
> +	ret = call; \
> +	toc_fixup(test); \
> +	ret; \
> +})
> +
> +#endif /* _POWERPC_STATIC_CALL_TEST_ */

On Mon, 2022-09-26 at 14:55 +0000, Christophe Leroy wrote:
> > +config PPC_STATIC_CALL_KUNIT_TEST
> > +       tristate "KUnit tests for PPC64 ELF ABI V2 static calls"
> > +       default KUNIT_ALL_TESTS
> > +       depends on HAVE_STATIC_CALL && PPC64_ELF_ABI_V2 && KUNIT &&
> > m
> 
> Is there a reason why it is dedicated to PPC64 ? In that case, can
> you 
> make it explicit with the name of the config option, and with the
> name 
> of the file below ?

The tests were written to make sure the TOC stays correct, so in theory
PPC64_ELF_ABI_V2 (and potentially PPC64_ELF_ABI_V1) is the only ABI
that should need them. I was thinking other tests should probably go in
static_call_selftest.c

Thinking now though, I suppose runtime modules are out-of-range for
branches on 32-bit as well? I can see it being useful for just testing
the indirect branch fallback in that case, without trying to make some
generic test suite that needs to work on other arches. The TOC specific
checks can be conditionally enabled per ABI.

Le 27/09/2022 à 05:31, Benjamin Gray a écrit :
> On Mon, 2022-09-26 at 14:55 +0000, Christophe Leroy wrote:
>>> +config PPC_STATIC_CALL_KUNIT_TEST
>>> +       tristate "KUnit tests for PPC64 ELF ABI V2 static calls"
>>> +       default KUNIT_ALL_TESTS
>>> +       depends on HAVE_STATIC_CALL && PPC64_ELF_ABI_V2 && KUNIT &&
>>> m
>>
>> Is there a reason why it is dedicated to PPC64 ? In that case, can
>> you
>> make it explicit with the name of the config option, and with the
>> name
>> of the file below ?
> 
> The tests were written to make sure the TOC stays correct, so in theory
> PPC64_ELF_ABI_V2 (and potentially PPC64_ELF_ABI_V1) is the only ABI
> that should need them. I was thinking other tests should probably go in
> static_call_selftest.c
> 
> Thinking now though, I suppose runtime modules are out-of-range for
> branches on 32-bit as well? I can see it being useful for just testing
> the indirect branch fallback in that case, without trying to make some
> generic test suite that needs to work on other arches. The TOC specific
> checks can be conditionally enabled per ABI.
> 

Yes it may happen that runtime modules are out-of-range, allthough we 
are doing our best to keep it as close as possible to kernel text, see 
commit 2ec13df16704 ("powerpc/modules: Load modules closer to kernel text")