Hi David,
On Sat, Nov 11, 2023 at 04:08:26AM +0800, David Gow wrote:
> KUnit's deferred action API accepts a void(*)(void *) function pointer
> which is called when the test is exited. However, we very frequently
> want to use existing functions which accept a single pointer, but which
> may not be of type void*. While this is probably dodgy enough to be on
> the wrong side of the C standard, it's been often used for similar
> callbacks, and gcc's -Wcast-function-type seems to ignore cases where
> the only difference is the type of the argument, assuming it's
> compatible (i.e., they're both pointers to data).
>
> However, clang 16 has introduced -Wcast-function-type-strict, which no
> longer permits any deviation in function pointer type. This seems to be
> because it'd break CFI, which validates the type of function calls.
>
> This rather ruins our attempts to cast functions to defer them, and
> leaves us with a few options. The one we've chosen is to implement a
> macro which will generate a wrapper function which accepts a void*, and
> casts the argument to the appropriate type.
>
> For example, if you were trying to wrap:
> void foo_close(struct foo *handle);
> you could use:
> KUNIT_DEFINE_ACTION_WRAPPER(kunit_action_foo_close,
> foo_close,
> struct foo *);
>
> This would create a new kunit_action_foo_close() function, of type
> kunit_action_t, which could be passed into kunit_add_action() and
> similar functions.
>
> In addition to defining this macro, update KUnit and its tests to use
> it.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1750
> Signed-off-by: David Gow <[email protected]>
> ---
>
> This is a follow-up to the RFC here:
> https://lore.kernel.org/linux-kselftest/[email protected]/
>
> There's no difference in the macro implementation, just an update to the
> KUnit tests to use it. This version is intended to complement:
> https://lore.kernel.org/all/[email protected]/
>
> There are also two follow-up patches in the series to use this macro in
> various DRM tests.
>
> Hopefully this will solve any CFI issues that show up with KUnit.
>
> Thanks,
> -- David
>
> ---
Prior to this series, there is indeed a crash when running the KUnit
tests with CONFIG_CFI_CLANG=y:
$ tools/testing/kunit/kunit.py run \
--alltests \
--arch x86_64 \
--kconfig_add CONFIG_CFI_CLANG=y \
--make_options LLVM=1 \
--timeout 30
...
[08:06:03] [ERROR] Test: sysctl_test: missing subtest result line!
[08:06:03] # module: sysctl_test
[08:06:03] 1..10
[08:06:03] CFI failure at __kunit_action_free+0x18/0x20 (target: kfree+0x0/0x80; expected type: 0xe82c6923)
[08:06:03] invalid opcode: 0000 [#1] PREEMPT NOPTI
[08:06:03] CPU: 0 PID: 53 Comm: kunit_try_catch Tainted: G N 6.7.0-rc1-00019-gc42d9eeef8e5 #3
[08:06:03] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-14-g1e1da7a96300-prebuilt.qemu.org 04/01/2014
[08:06:03] RIP: 0010:__kunit_action_free+0x18/0x20
[08:06:03] Code: 00 00 b8 ae 55 f1 4d 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 4c 8b 5f 38 48 8b 7f 40 41 ba dd 96 d3 17 45 03 53 f1 74 02 <0f> 0b 2e e9 f0 b5 46 00 b8 fa f1 06 5e 90 90 90 90 90 90 90 90 90
[08:06:03] RSP: 0018:ffffb0d2c00ebea0 EFLAGS: 00000292
[08:06:03] RAX: 0000000000000001 RBX: ffff993d41949a80 RCX: ffff993d41949aa0
[08:06:03] RDX: 0000000000000282 RSI: ffff993d41949a80 RDI: ffff993d4186b6b0
[08:06:03] RBP: ffffb0d2c0013ad8 R08: ffffffffc9c84000 R09: 0000000000000400
[08:06:03] R10: 00000000f707d502 R11: ffffffff8f33aa40 R12: ffff993d418d2e00
[08:06:03] R13: ffff993d41a05600 R14: ffffb0d2c0013cc0 R15: ffff993d41949ae0
[08:06:03] FS: 0000000000000000(0000) GS:ffffffff90049000(0000) knlGS:0000000000000000
[08:06:03] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[08:06:03] CR2: ffff993d55c01000 CR3: 000000001563e000 CR4: 00000000000006f0
[08:06:03] Call Trace:
[08:06:03] <TASK>
[08:06:03] ? __die+0xd6/0x120
[08:06:03] ? die+0x5f/0xa0
[08:06:03] ? do_trap+0x9b/0x180
[08:06:03] ? __kunit_action_free+0x18/0x20
[08:06:03] ? __kunit_action_free+0x18/0x20
[08:06:03] ? handle_invalid_op+0x64/0x80
[08:06:03] ? __kunit_action_free+0x18/0x20
[08:06:03] ? exc_invalid_op+0x38/0x60
[08:06:03] ? asm_exc_invalid_op+0x1a/0x20
[08:06:03] ? __cfi_kfree+0x10/0x10
[08:06:03] ? __kunit_action_free+0x18/0x20
[08:06:03] kunit_remove_resource+0x8f/0xf0
[08:06:03] kunit_cleanup+0x60/0xe0
[08:06:03] kunit_generic_run_threadfn_adapter+0x24/0x30
[08:06:03] ? __cfi_kunit_generic_run_threadfn_adapter+0x10/0x10
[08:06:03] kthread+0xd9/0xf0
[08:06:03] ? __cfi_kthread+0x10/0x10
[08:06:03] ret_from_fork+0x43/0x50
[08:06:03] ? __cfi_kthread+0x10/0x10
[08:06:03] ret_from_fork_asm+0x1a/0x30
[08:06:03] </TASK>
[08:06:03] ---[ end trace 0000000000000000 ]---
[08:06:03] RIP: 0010:__kunit_action_free+0x18/0x20
...
With this series applied with
https://lore.kernel.org/[email protected]/,
all the tests pass for arm64 and x86_64 on my machine. I see no
remaining casts in the tree in this state. It seems like the
documentation in Documentation/dev-tools/kunit/usage.rst may want to be
updated to remove mention of casting to kunit_action_t as well?
Regardless:
Reviewed-by: Nathan Chancellor <[email protected]>
Tested-by: Nathan Chancellor <[email protected]>
> include/kunit/resource.h | 9 +++++++++
> lib/kunit/kunit-test.c | 5 +----
> lib/kunit/test.c | 6 ++++--
> 3 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/include/kunit/resource.h b/include/kunit/resource.h
> index c7383e90f5c9..4110e13970dc 100644
> --- a/include/kunit/resource.h
> +++ b/include/kunit/resource.h
> @@ -390,6 +390,15 @@ void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);
> /* A 'deferred action' function to be used with kunit_add_action. */
> typedef void (kunit_action_t)(void *);
>
> +/* We can't cast function pointers to kunit_action_t if CFI is enabled. */
> +#define KUNIT_DEFINE_ACTION_WRAPPER(wrapper, orig, arg_type) \
> + static void wrapper(void *in) \
> + { \
> + arg_type arg = (arg_type)in; \
> + orig(arg); \
> + }
> +
> +
> /**
> * kunit_add_action() - Call a function when the test ends.
> * @test: Test case to associate the action with.
> diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> index de2113a58fa0..ee6927c60979 100644
> --- a/lib/kunit/kunit-test.c
> +++ b/lib/kunit/kunit-test.c
> @@ -538,10 +538,7 @@ static struct kunit_suite kunit_resource_test_suite = {
> #if IS_BUILTIN(CONFIG_KUNIT_TEST)
>
> /* This avoids a cast warning if kfree() is passed direct to kunit_add_action(). */
> -static void kfree_wrapper(void *p)
> -{
> - kfree(p);
> -}
> +KUNIT_DEFINE_ACTION_WRAPPER(kfree_wrapper, kfree, const void *);
>
> static void kunit_log_test(struct kunit *test)
> {
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index f2eb71f1a66c..0308865194bb 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -772,6 +772,8 @@ static struct notifier_block kunit_mod_nb = {
> };
> #endif
>
> +KUNIT_DEFINE_ACTION_WRAPPER(kfree_action_wrapper, kfree, const void *)
> +
> void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t gfp)
> {
> void *data;
> @@ -781,7 +783,7 @@ void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t gfp)
> if (!data)
> return NULL;
>
> - if (kunit_add_action_or_reset(test, (kunit_action_t *)kfree, data) != 0)
> + if (kunit_add_action_or_reset(test, kfree_action_wrapper, data) != 0)
> return NULL;
>
> return data;
> @@ -793,7 +795,7 @@ void kunit_kfree(struct kunit *test, const void *ptr)
> if (!ptr)
> return;
>
> - kunit_release_action(test, (kunit_action_t *)kfree, (void *)ptr);
> + kunit_release_action(test, kfree_action_wrapper, (void *)ptr);
> }
> EXPORT_SYMBOL_GPL(kunit_kfree);
>
> --
> 2.42.0.869.gea05f2083d-goog
>