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 davidgow@google.com ---
This is a follow-up to the RFC here: https://lore.kernel.org/linux-kselftest/20230915050125.3609689-1-davidgow@go...
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/20231106172557.2963-1-rf@opensource.cirrus.com/
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
--- 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);
In order to pass functions to kunit_add_action(), they need to be of the kunit_action_t type. While casting the function pointer can work, it will break control-flow integrity.
drm_kunit_helpers already defines wrappers, but we now have a macro which does this automatically. Using this greatly reduces the boilerplate needed.
Signed-off-by: David Gow davidgow@google.com ---
This patch should be a no-op, just moving to use a standard macro to implement these wrappers rather than hand-coding them.
Let me know if you'd prefer to take these in separately via the drm trees, or if you're okay with having this whole series go via kselftest/kunit.
Cheers, -- David
--- drivers/gpu/drm/tests/drm_kunit_helpers.c | 30 +++++++---------------- 1 file changed, 9 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c index bccb33b900f3..c251e6b34de0 100644 --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c @@ -27,27 +27,15 @@ static struct platform_driver fake_platform_driver = { }, };
-static void kunit_action_platform_driver_unregister(void *ptr) -{ - struct platform_driver *drv = ptr; - - platform_driver_unregister(drv); - -} - -static void kunit_action_platform_device_put(void *ptr) -{ - struct platform_device *pdev = ptr; - - platform_device_put(pdev); -} - -static void kunit_action_platform_device_del(void *ptr) -{ - struct platform_device *pdev = ptr; - - platform_device_del(pdev); -} +KUNIT_DEFINE_ACTION_WRAPPER(kunit_action_platform_driver_unregister, + platform_driver_unregister, + struct platform_driver *); +KUNIT_DEFINE_ACTION_WRAPPER(kunit_action_platform_device_put, + platform_device_put, + struct platform_device *); +KUNIT_DEFINE_ACTION_WRAPPER(kunit_action_platform_device_del, + platform_device_del, + struct platform_device *);
/** * drm_kunit_helper_alloc_device - Allocate a mock device for a KUnit test
Hi David,
On Sat, Nov 11, 2023 at 04:08:27AM +0800, David Gow wrote:
In order to pass functions to kunit_add_action(), they need to be of the kunit_action_t type. While casting the function pointer can work, it will break control-flow integrity.
drm_kunit_helpers already defines wrappers, but we now have a macro which does this automatically. Using this greatly reduces the boilerplate needed.
Signed-off-by: David Gow davidgow@google.com
This patch should be a no-op, just moving to use a standard macro to implement these wrappers rather than hand-coding them.
Let me know if you'd prefer to take these in separately via the drm trees, or if you're okay with having this whole series go via kselftest/kunit.
You can merge it through your tree with
Acked-by: Maxime Ripard mripard@kernel.org
For the patches 2 and 3
Maxime
In order to pass functions to kunit_add_action(), they need to be of the kunit_action_t type. While casting the function pointer can work, it will break control-flow integrity.
vc4_mock already defines such a wrapper for drm_dev_unregister(), but it involves less boilerplate to use the new macro, so replace the manual implementation.
Signed-off-by: David Gow davidgow@google.com ---
This patch should be a no-op, just moving to use a standard macro to implement these wrappers rather than hand-coding them.
Let me know if you'd prefer to take these in separately via the drm trees, or if you're okay with having this whole series go via kselftest/kunit.
Cheers, -- David
--- drivers/gpu/drm/vc4/tests/vc4_mock.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock.c b/drivers/gpu/drm/vc4/tests/vc4_mock.c index 63ca46f4cb35..becb3dbaa548 100644 --- a/drivers/gpu/drm/vc4/tests/vc4_mock.c +++ b/drivers/gpu/drm/vc4/tests/vc4_mock.c @@ -153,12 +153,9 @@ static int __build_mock(struct kunit *test, struct drm_device *drm, return 0; }
-static void kunit_action_drm_dev_unregister(void *ptr) -{ - struct drm_device *drm = ptr; - - drm_dev_unregister(drm); -} +KUNIT_DEFINE_ACTION_WRAPPER(kunit_action_drm_dev_unregister, + drm_dev_unregister, + struct drm_device *);
static struct vc4_dev *__mock_device(struct kunit *test, bool is_vc5) {
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 davidgow@google.com
This is a follow-up to the RFC here: https://lore.kernel.org/linux-kselftest/20230915050125.3609689-1-davidgow@go...
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/20231106172557.2963-1-rf@opensource.cirrus.com/
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/20231106172557.2963-1-rf@opensource.cirrus.com/, 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 nathan@kernel.org Tested-by: Nathan Chancellor nathan@kernel.org
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
On Sat, 11 Nov 2023 04:08:26 +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
[ ... ]
Reviewed-by: Maxime Ripard mripard@kernel.org
Thanks! Maxime
On Wed, 15 Nov 2023 at 16:51, Maxime Ripard mripard@kernel.org wrote:
On Sat, 11 Nov 2023 04:08:26 +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
[ ... ]
Reviewed-by: Maxime Ripard mripard@kernel.org
Acked-by: Daniel Vetter daniel@ffwll.ch for merging through kunit tree, since I guess that's the simplest way to land this.
Cheers!
linux-kselftest-mirror@lists.linaro.org