All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Gow <davidgow@google.com>
To: Nathan Chancellor <nathan@kernel.org>,
	Kees Cook <keescook@chromium.org>,
	 Brendan Higgins <brendan.higgins@linux.dev>,
	Rae Moar <rmoar@google.com>,
	dlatypov@google.com,  Maxime Ripard <mripard@kernel.org>,
	Arthur Grillo <arthurgrillo@riseup.net>,
	 Shuah Khan <skhan@linuxfoundation.org>
Cc: "David Gow" <davidgow@google.com>,
	"Maíra Canal" <mairacanal@riseup.net>,
	"Sami Tolvanen" <samitolvanen@google.com>,
	kunit-dev@googlegroups.com, llvm@lists.linux.dev,
	linux-hardening@vger.kernel.org, linux-kselftest@vger.kernel.org,
	"Benjamin Berg" <benjamin.berg@intel.com>,
	"Richard Fitzgerald" <rf@opensource.cirrus.com>,
	linux-kernel@vger.kernel.org,
	"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	"Emma Anholt" <emma@anholt.net>,
	"David Airlie" <airlied@gmail.com>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	dri-devel@lists.freedesktop.org
Subject: [PATCH 1/3] kunit: Add a macro to wrap a deferred action function
Date: Sat, 11 Nov 2023 04:08:26 +0800	[thread overview]
Message-ID: <20231110200830.1832556-1-davidgow@google.com> (raw)

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@google.com/

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);
 
-- 
2.42.0.869.gea05f2083d-goog


WARNING: multiple messages have this Message-ID (diff)
From: David Gow <davidgow@google.com>
To: Nathan Chancellor <nathan@kernel.org>,
	Kees Cook <keescook@chromium.org>,
	 Brendan Higgins <brendan.higgins@linux.dev>,
	Rae Moar <rmoar@google.com>,
	dlatypov@google.com,  Maxime Ripard <mripard@kernel.org>,
	Arthur Grillo <arthurgrillo@riseup.net>,
	 Shuah Khan <skhan@linuxfoundation.org>
Cc: linux-kselftest@vger.kernel.org,
	"David Gow" <davidgow@google.com>,
	"Emma Anholt" <emma@anholt.net>,
	"Benjamin Berg" <benjamin.berg@intel.com>,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	llvm@lists.linux.dev, linux-kernel@vger.kernel.org,
	dri-devel@lists.freedesktop.org,
	"Maíra Canal" <mairacanal@riseup.net>,
	"Richard Fitzgerald" <rf@opensource.cirrus.com>,
	linux-hardening@vger.kernel.org,
	"Sami Tolvanen" <samitolvanen@google.com>,
	kunit-dev@googlegroups.com
Subject: [PATCH 1/3] kunit: Add a macro to wrap a deferred action function
Date: Sat, 11 Nov 2023 04:08:26 +0800	[thread overview]
Message-ID: <20231110200830.1832556-1-davidgow@google.com> (raw)

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@google.com/

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);
 
-- 
2.42.0.869.gea05f2083d-goog


             reply	other threads:[~2023-11-10 20:08 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-10 20:08 David Gow [this message]
2023-11-10 20:08 ` [PATCH 1/3] kunit: Add a macro to wrap a deferred action function David Gow
2023-11-10 20:08 ` [PATCH 2/3] drm/tests: Use KUNIT_DEFINE_ACTION_WRAPPER() David Gow
2023-11-10 20:08   ` David Gow
2023-11-15 15:50   ` Maxime Ripard
2023-11-15 15:50     ` Maxime Ripard
2023-11-10 20:08 ` [PATCH 3/3] drm/vc4: tests: Use KUNIT_DEFINE_ACTION_WRAPPER David Gow
2023-11-10 20:08   ` David Gow
2023-11-15 15:23 ` [PATCH 1/3] kunit: Add a macro to wrap a deferred action function Nathan Chancellor
2023-11-15 15:23   ` Nathan Chancellor
2023-11-15 15:51 ` Maxime Ripard
2023-11-15 15:51   ` Maxime Ripard
2023-11-15 16:00   ` Daniel Vetter
2023-11-15 16:00     ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231110200830.1832556-1-davidgow@google.com \
    --to=davidgow@google.com \
    --cc=airlied@gmail.com \
    --cc=arthurgrillo@riseup.net \
    --cc=benjamin.berg@intel.com \
    --cc=brendan.higgins@linux.dev \
    --cc=daniel@ffwll.ch \
    --cc=dlatypov@google.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=emma@anholt.net \
    --cc=keescook@chromium.org \
    --cc=kunit-dev@googlegroups.com \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=llvm@lists.linux.dev \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=mairacanal@riseup.net \
    --cc=mripard@kernel.org \
    --cc=nathan@kernel.org \
    --cc=rf@opensource.cirrus.com \
    --cc=rmoar@google.com \
    --cc=samitolvanen@google.com \
    --cc=skhan@linuxfoundation.org \
    --cc=tzimmermann@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.