[PATCH 0/2] Define is_signed_type() once

[PATCH 0/2] Define is_signed_type() once

[Bart Van Assche]

Hi Kees,

The changes in this patch series are as follows:
- Add a unit test for the is_signed_type() macro.
- Define the is_signed_type() macro once.

Please consider these patches for the next merge window.

Thanks,

Bart.

Bart Van Assche (2):
  testing/selftests: Add tests for the is_signed_type() macro
  overflow, tracing: Define the is_signed_type() macro once

 include/linux/compiler.h     |  6 +++++
 include/linux/overflow.h     |  1 -
 include/linux/trace_events.h |  2 --
 lib/Kconfig.debug            | 12 +++++++++
 lib/Makefile                 |  1 +
 lib/is_signed_type_test.c    | 48 ++++++++++++++++++++++++++++++++++++
 6 files changed, 67 insertions(+), 3 deletions(-)
 create mode 100644 lib/is_signed_type_test.c

[PATCH 1/2] testing/selftests: Add tests for the is_signed_type() macro

[Bart Van Assche]

Although not documented, is_signed_type() must support the 'bool' and
pointer types next to scalar and enumeration types. Add a selftest that
verifies that this macro handles all supported types correctly.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Isabella Basso <isabbasso@riseup.net>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Sander Vanheule <sander@svanheule.net>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Yury Norov <yury.norov@gmail.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 lib/Kconfig.debug         | 12 ++++++++++
 lib/Makefile              |  1 +
 lib/is_signed_type_test.c | 48 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+)
 create mode 100644 lib/is_signed_type_test.c

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 072e4b289c13..36455953d306 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2506,6 +2506,18 @@ config MEMCPY_KUNIT_TEST
 
 	  If unsure, say N.
 
+config IS_SIGNED_TYPE_KUNIT_TEST
+	tristate "Test is_signed_type() macro" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  Builds unit tests for the is_signed_type() macro.
+
+	  For more information on KUnit and unit tests in general please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.
+
 config OVERFLOW_KUNIT_TEST
 	tristate "Test check_*_overflow() functions at runtime" if !KUNIT_ALL_TESTS
 	depends on KUNIT
diff --git a/lib/Makefile b/lib/Makefile
index 5927d7fa0806..70176ff17023 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -377,6 +377,7 @@ obj-$(CONFIG_BITS_TEST) += test_bits.o
 obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
 obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o
 obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o
+obj-$(CONFIG_IS_SIGNED_TYPE_KUNIT_TEST) += is_signed_type_test.o
 obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o
 CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable)
 obj-$(CONFIG_STACKINIT_KUNIT_TEST) += stackinit_kunit.o
diff --git a/lib/is_signed_type_test.c b/lib/is_signed_type_test.c
new file mode 100644
index 000000000000..f2eedb1f0935
--- /dev/null
+++ b/lib/is_signed_type_test.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ *	./tools/testing/kunit/kunit.py run is_signed_type [--raw_output]
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <kunit/test.h>
+#include <linux/overflow.h>
+
+enum unsigned_enum {
+	constant_a = 3,
+};
+
+enum signed_enum {
+	constant_b = -1,
+	constant_c = 2,
+};
+
+static void is_signed_type_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, is_signed_type(bool), false);
+	KUNIT_EXPECT_EQ(test, is_signed_type(signed char), true);
+	KUNIT_EXPECT_EQ(test, is_signed_type(unsigned char), false);
+	KUNIT_EXPECT_EQ(test, is_signed_type(int), true);
+	KUNIT_EXPECT_EQ(test, is_signed_type(unsigned int), false);
+	KUNIT_EXPECT_EQ(test, is_signed_type(long), true);
+	KUNIT_EXPECT_EQ(test, is_signed_type(unsigned long), false);
+	KUNIT_EXPECT_EQ(test, is_signed_type(long long), true);
+	KUNIT_EXPECT_EQ(test, is_signed_type(unsigned long long), false);
+	KUNIT_EXPECT_EQ(test, is_signed_type(enum unsigned_enum), false);
+	KUNIT_EXPECT_EQ(test, is_signed_type(enum signed_enum), true);
+	KUNIT_EXPECT_EQ(test, is_signed_type(void *), false);
+	KUNIT_EXPECT_EQ(test, is_signed_type(const char *), false);
+}
+
+static struct kunit_case is_signed_type_test_cases[] = {
+	KUNIT_CASE(is_signed_type_test),
+	{}
+};
+
+static struct kunit_suite is_signed_type_test_suite = {
+	.name = "is_signed_type",
+	.test_cases = is_signed_type_test_cases,
+};
+
+kunit_test_suite(is_signed_type_test_suite);
+
+MODULE_LICENSE("Dual MIT/GPL");

[PATCH 2/2] overflow, tracing: Define the is_signed_type() macro once

[Bart Van Assche]

There are two definitions of the is_signed_type() macro: one in
<linux/overflow.h> and a second definition in <linux/trace_events.h>.

As suggested by Linus Torvalds, move the definition of the
is_signed_type() macro into the <linux/compiler.h> header file. Change
the definition of the is_signed_type() macro to make sure that it does
not trigger any sparse warnings with future versions of sparse for
bitwise types. See also:
https://lore.kernel.org/all/CAHk-=whjH6p+qzwUdx5SOVVHjS3WvzJQr6mDUwhEyTf6pJWzaQ@mail.gmail.com/
https://lore.kernel.org/all/CAHk-=wjQGnVfb4jehFR0XyZikdQvCZouE96xR_nnf5kqaM5qqQ@mail.gmail.com/

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Isabella Basso <isabbasso@riseup.net>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Sander Vanheule <sander@svanheule.net>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Yury Norov <yury.norov@gmail.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/linux/compiler.h     | 6 ++++++
 include/linux/overflow.h     | 1 -
 include/linux/trace_events.h | 2 --
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 01ce94b58b42..7713d7bcdaea 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -239,6 +239,12 @@ static inline void *offset_to_ptr(const int *off)
 /* &a[0] degrades to a pointer: a different type from an array */
 #define __must_be_array(a)	BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
 
+/*
+ * Whether 'type' is a signed type or an unsigned type. Supports scalar types,
+ * bool and also pointer types.
+ */
+#define is_signed_type(type) (((type)(-1)) < (__force type)1)
+
 /*
  * This is needed in functions which generate the stack canary, see
  * arch/x86/kernel/smpboot.c::start_secondary() for an example.
diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index f1221d11f8e5..0eb3b192f07a 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -30,7 +30,6 @@
  * https://mail-index.netbsd.org/tech-misc/2007/02/05/0000.html -
  * credit to Christian Biere.
  */
-#define is_signed_type(type)       (((type)(-1)) < (type)1)
 #define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 - is_signed_type(type)))
 #define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T)))
 #define type_min(T) ((T)((T)-type_max(T)-(T)1))
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index b18759a673c6..8401dec93c15 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -814,8 +814,6 @@ extern int trace_add_event_call(struct trace_event_call *call);
 extern int trace_remove_event_call(struct trace_event_call *call);
 extern int trace_event_get_offsets(struct trace_event_call *call);
 
-#define is_signed_type(type)	(((type)(-1)) < (type)1)
-
 int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);
 int trace_set_clr_event(const char *system, const char *event, int set);
 int trace_array_set_clr_event(struct trace_array *tr, const char *system,

Re: [PATCH 1/2] testing/selftests: Add tests for the is_signed_type() macro

[Kees Cook]

On Fri, Aug 26, 2022 at 09:21:15AM -0700, Bart Van Assche wrote:
> Although not documented, is_signed_type() must support the 'bool' and
> pointer types next to scalar and enumeration types. Add a selftest that
> verifies that this macro handles all supported types correctly.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Isabella Basso <isabbasso@riseup.net>
> Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
> Cc: Josh Poimboeuf <jpoimboe@kernel.org>
> Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Sander Vanheule <sander@svanheule.net>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Yury Norov <yury.norov@gmail.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 2/2] overflow, tracing: Define the is_signed_type() macro once

[Kees Cook]

On Fri, Aug 26, 2022 at 09:21:16AM -0700, Bart Van Assche wrote:
> There are two definitions of the is_signed_type() macro: one in
> <linux/overflow.h> and a second definition in <linux/trace_events.h>.
> 
> As suggested by Linus Torvalds, move the definition of the
> is_signed_type() macro into the <linux/compiler.h> header file. Change
> the definition of the is_signed_type() macro to make sure that it does
> not trigger any sparse warnings with future versions of sparse for
> bitwise types. See also:
> https://lore.kernel.org/all/CAHk-=whjH6p+qzwUdx5SOVVHjS3WvzJQr6mDUwhEyTf6pJWzaQ@mail.gmail.com/
> https://lore.kernel.org/all/CAHk-=wjQGnVfb4jehFR0XyZikdQvCZouE96xR_nnf5kqaM5qqQ@mail.gmail.com/
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Isabella Basso <isabbasso@riseup.net>
> Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
> Cc: Josh Poimboeuf <jpoimboe@kernel.org>
> Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Sander Vanheule <sander@svanheule.net>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Yury Norov <yury.norov@gmail.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  include/linux/compiler.h     | 6 ++++++
>  include/linux/overflow.h     | 1 -
>  include/linux/trace_events.h | 2 --
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 01ce94b58b42..7713d7bcdaea 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -239,6 +239,12 @@ static inline void *offset_to_ptr(const int *off)
>  /* &a[0] degrades to a pointer: a different type from an array */
>  #define __must_be_array(a)	BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
>  
> +/*
> + * Whether 'type' is a signed type or an unsigned type. Supports scalar types,
> + * bool and also pointer types.
> + */
> +#define is_signed_type(type) (((type)(-1)) < (__force type)1)
> +
>  /*
>   * This is needed in functions which generate the stack canary, see
>   * arch/x86/kernel/smpboot.c::start_secondary() for an example.
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index f1221d11f8e5..0eb3b192f07a 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -30,7 +30,6 @@
>   * https://mail-index.netbsd.org/tech-misc/2007/02/05/0000.html -
>   * credit to Christian Biere.
>   */
> -#define is_signed_type(type)       (((type)(-1)) < (type)1)
>  #define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 - is_signed_type(type)))
>  #define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T)))
>  #define type_min(T) ((T)((T)-type_max(T)-(T)1))
> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> index b18759a673c6..8401dec93c15 100644
> --- a/include/linux/trace_events.h
> +++ b/include/linux/trace_events.h
> @@ -814,8 +814,6 @@ extern int trace_add_event_call(struct trace_event_call *call);
>  extern int trace_remove_event_call(struct trace_event_call *call);
>  extern int trace_event_get_offsets(struct trace_event_call *call);
>  
> -#define is_signed_type(type)	(((type)(-1)) < (type)1)
> -
>  int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);
>  int trace_set_clr_event(const char *system, const char *event, int set);
>  int trace_array_set_clr_event(struct trace_array *tr, const char *system,

Yeah, this looks good. I'll take these as part of the hardening tree
since it's touching overflow.h, unless I hear otherwise. :)

-- 
Kees Cook

Re: [PATCH 0/2] Define is_signed_type() once

[Kees Cook]

On Fri, 26 Aug 2022 09:21:14 -0700, Bart Van Assche wrote:
> The changes in this patch series are as follows:
> - Add a unit test for the is_signed_type() macro.
> - Define the is_signed_type() macro once.
> 
> Please consider these patches for the next merge window.
> 
> Thanks,
> 
> [...]

Applied to for-next/hardening, thanks!

[1/2] testing/selftests: Add tests for the is_signed_type() macro
      https://git.kernel.org/kees/c/5e3ad11bfc5a
[2/2] overflow, tracing: Define the is_signed_type() macro once
      https://git.kernel.org/kees/c/6bf7edc1e6f0

I tweaked the kunit test to have a matching filename to the other kunit
tests (i.e. ending with _kunit.c instead of _test.c)

-- 
Kees Cook

Re: [PATCH 1/2] testing/selftests: Add tests for the is_signed_type() macro

[Isabella Basso]

Hi, Bart,

Glad to see some more KUnit tests making it inside of the kernel’s lib folder :).

> Am 26/08/2022 um 1:21 PM schrieb Bart Van Assche <bvanassche@acm.org>:
> 
> Although not documented, is_signed_type() must support the 'bool' and
> pointer types next to scalar and enumeration types. Add a selftest that
> verifies that this macro handles all supported types correctly.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Isabella Basso <isabbasso@riseup.net>
> Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
> Cc: Josh Poimboeuf <jpoimboe@kernel.org>
> Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Sander Vanheule <sander@svanheule.net>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Yury Norov <yury.norov@gmail.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> lib/Kconfig.debug         | 12 ++++++++++
> lib/Makefile              |  1 +
> lib/is_signed_type_test.c | 48 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 61 insertions(+)
> create mode 100644 lib/is_signed_type_test.c
> 
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 072e4b289c13..36455953d306 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2506,6 +2506,18 @@ config MEMCPY_KUNIT_TEST
> 
> 	  If unsure, say N.
> 
> +config IS_SIGNED_TYPE_KUNIT_TEST
> +	tristate "Test is_signed_type() macro" if !KUNIT_ALL_TESTS
> +	depends on KUNIT
> +	default KUNIT_ALL_TESTS
> +	help
> +	  Builds unit tests for the is_signed_type() macro.
> +
> +	  For more information on KUnit and unit tests in general please refer
> +	  to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> +	  If unsure, say N.
> +
> config OVERFLOW_KUNIT_TEST
> 	tristate "Test check_*_overflow() functions at runtime" if !KUNIT_ALL_TESTS
> 	depends on KUNIT
> diff --git a/lib/Makefile b/lib/Makefile
> index 5927d7fa0806..70176ff17023 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -377,6 +377,7 @@ obj-$(CONFIG_BITS_TEST) += test_bits.o
> obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
> obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o
> obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o
> +obj-$(CONFIG_IS_SIGNED_TYPE_KUNIT_TEST) += is_signed_type_test.o
> obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o
> CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable)
> obj-$(CONFIG_STACKINIT_KUNIT_TEST) += stackinit_kunit.o
> diff --git a/lib/is_signed_type_test.c b/lib/is_signed_type_test.c
> new file mode 100644
> index 000000000000..f2eedb1f0935
> --- /dev/null
> +++ b/lib/is_signed_type_test.c
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + *	./tools/testing/kunit/kunit.py run is_signed_type [--raw_output]
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <kunit/test.h>
> +#include <linux/overflow.h>

Nit: I don’t know if that makes a huge difference but you might include
`<linux/compiler.h>` directly to make the final object smaller. Of course, that
would ideally be a change happening in 2/2 but that was already merged :).

> +
> +enum unsigned_enum {
> +	constant_a = 3,
> +};
> +
> +enum signed_enum {
> +	constant_b = -1,
> +	constant_c = 2,
> +};
> +
> +static void is_signed_type_test(struct kunit *test)
> +{
> +	KUNIT_EXPECT_EQ(test, is_signed_type(bool), false);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(signed char), true);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(unsigned char), false);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(int), true);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(unsigned int), false);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(long), true);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(unsigned long), false);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(long long), true);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(unsigned long long), false);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(enum unsigned_enum), false);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(enum signed_enum), true);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(void *), false);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(const char *), false);
> +}
> +
> +static struct kunit_case is_signed_type_test_cases[] = {
> +	KUNIT_CASE(is_signed_type_test),
> +	{}
> +};
> +
> +static struct kunit_suite is_signed_type_test_suite = {
> +	.name = "is_signed_type",
> +	.test_cases = is_signed_type_test_cases,
> +};
> +
> +kunit_test_suite(is_signed_type_test_suite);
> +
> +MODULE_LICENSE("Dual MIT/GPL“);

Tested-by: Isabella Basso <isabbasso@riseup.net>

Cheers,
--
Isabella Basso

Re: [PATCH 1/2] testing/selftests: Add tests for the is_signed_type() macro

[Bart Van Assche]

On 8/29/22 19:33, Isabella Basso wrote:
>> +#include <kunit/test.h>
>> +#include <linux/overflow.h>
> 
> Nit: I don’t know if that makes a huge difference but you might include
> `<linux/compiler.h>` directly to make the final object smaller. Of course, that
> would ideally be a change happening in 2/2 but that was already merged :).

Right, that could have been done in patch 2/2 but I think this can also 
be done as a follow-up patch. I'm not sure what Kees prefers?

Thanks,

Bart.

Re: [PATCH 1/2] testing/selftests: Add tests for the is_signed_type() macro

[Rasmus Villemoes]

On 26/08/2022 18.21, Bart Van Assche wrote:
> Although not documented, is_signed_type() must support the 'bool' and
> pointer types next to scalar and enumeration types. Add a selftest that
> verifies that this macro handles all supported types correctly.
> 

> +static void is_signed_type_test(struct kunit *test)
> +{
> +	KUNIT_EXPECT_EQ(test, is_signed_type(bool), false);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(signed char), true);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(unsigned char), false);

Nice. You could consider adding

#ifdef __UNSIGNED_CHAR__
KUNIT_EXPECT_EQ(test, is_signed_type(char), false);
#else
KUNIT_EXPECT_EQ(test, is_signed_type(char), true);
#endif

The kernel depends on the compiler providing __UNSIGNED_CHAR__ in two
places (one in ext4, one in printf test suite).

> +	KUNIT_EXPECT_EQ(test, is_signed_type(int), true);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(unsigned int), false);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(long), true);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(unsigned long), false);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(long long), true);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(unsigned long long), false);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(enum unsigned_enum), false);
> +	KUNIT_EXPECT_EQ(test, is_signed_type(enum signed_enum), true);

Yeah. But enum types are one of the weirdest aspects of C. Taking your
example and expanding with a positive value that doesn't fit an int:

#include <stdio.h>

#define is_signed_type(t) ((t)-1 < (t)1)

#define typeinfo(t) printf("%-24s signed %d, size %zu\n", #t,
is_signed_type(t), sizeof(t))

enum unsigned_enum {
	constant_a = 3,
	constant_d = 3000000000,
};

enum signed_enum {
	constant_b = -1,
	constant_c = 2,
};

int main(int argc, char *argv[])
{
	enum unsigned_enum a = constant_a;
	enum unsigned_enum d = constant_d;
	enum signed_enum b = constant_b;
	enum signed_enum c = constant_c;

	typeinfo(enum unsigned_enum);
	typeinfo(enum signed_enum);
	typeinfo(typeof(constant_a));
	typeinfo(typeof(constant_b));
	typeinfo(typeof(constant_c));
	typeinfo(typeof(constant_d));

	typeinfo(typeof(a));
	typeinfo(typeof(b));
	typeinfo(typeof(c));
	typeinfo(typeof(d));

	return 0;
}

This gives me

enum unsigned_enum       signed 0, size 4
enum signed_enum         signed 1, size 4
typeof(constant_a)       signed 1, size 4
typeof(constant_b)       signed 1, size 4
typeof(constant_c)       signed 1, size 4
typeof(constant_d)       signed 0, size 4
typeof(a)                signed 0, size 4
typeof(b)                signed 1, size 4
typeof(c)                signed 1, size 4
typeof(d)                signed 0, size 4

That is, typeof(constant_a) is not the same type (different signedness)
as enum unsigned_enum! While both constant_d (due to its size) and
variables declared as 'enum unsigned_enum' do indeed have that
underlying unsigned type.

At least gcc and clang agree on this weirdness, but I haven't been able
to find a spec mandating this. Anyway, this was just an aside.

Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Re: [PATCH 1/2] testing/selftests: Add tests for the is_signed_type() macro

[Kees Cook]

On Mon, Aug 29, 2022 at 08:30:54PM -0700, Bart Van Assche wrote:
> On 8/29/22 19:33, Isabella Basso wrote:
> > > +#include <kunit/test.h>
> > > +#include <linux/overflow.h>
> > 
> > Nit: I don’t know if that makes a huge difference but you might include
> > `<linux/compiler.h>` directly to make the final object smaller. Of course, that
> > would ideally be a change happening in 2/2 but that was already merged :).
> 
> Right, that could have been done in patch 2/2 but I think this can also be
> done as a follow-up patch. I'm not sure what Kees prefers?

A follow-up would be easier for me. And perhaps could include Rasmus's
suggestions too?

-- 
Kees Cook

Re: [PATCH 1/2] testing/selftests: Add tests for the is_signed_type() macro

[Bart Van Assche]

On 8/30/22 02:41, Rasmus Villemoes wrote:
> On 26/08/2022 18.21, Bart Van Assche wrote:
>> Although not documented, is_signed_type() must support the 'bool' and
>> pointer types next to scalar and enumeration types. Add a selftest that
>> verifies that this macro handles all supported types correctly.
>>
> 
>> +static void is_signed_type_test(struct kunit *test)
>> +{
>> +	KUNIT_EXPECT_EQ(test, is_signed_type(bool), false);
>> +	KUNIT_EXPECT_EQ(test, is_signed_type(signed char), true);
>> +	KUNIT_EXPECT_EQ(test, is_signed_type(unsigned char), false);
> 
> Nice. You could consider adding
> 
> #ifdef __UNSIGNED_CHAR__
> KUNIT_EXPECT_EQ(test, is_signed_type(char), false);
> #else
> KUNIT_EXPECT_EQ(test, is_signed_type(char), true);
> #endif
> 
> The kernel depends on the compiler providing __UNSIGNED_CHAR__ in two
> places (one in ext4, one in printf test suite).

(reduced Cc-list)

Hi Rasmus,

Since I would like to implement the above suggestion I tried to look up 
other uses of the __UNSIGNED_CHAR__ macro. However, I couldn't find any. 
Did I perhaps do something wrong?
  $ git grep -w __UNSIGNED_CHAR__ origin/master | wc
       0       0       0

Thanks,

Bart.

Re: [PATCH 1/2] testing/selftests: Add tests for the is_signed_type() macro

[Rasmus Villemoes]

On 07/09/2022 00.42, Bart Van Assche wrote:

> Since I would like to implement the above suggestion I tried to look up
> other uses of the __UNSIGNED_CHAR__ macro. However, I couldn't find any.
> Did I perhaps do something wrong?
>  $ git grep -w __UNSIGNED_CHAR__ origin/master | wc
>       0       0       0

No, sorry, I did. It's __CHAR_UNSIGNED__ . Here's the description from
'info cpp':

'__CHAR_UNSIGNED__'
     GCC defines this macro if and only if the data type 'char' is
     unsigned on the target machine.

Rasmus