linux-kernel - Re: [PATCH] selftests: harness: Rework is_signed_type() to avoid collision with overflow.h

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <fa9e2fae-012a-4a34-883b-30c4c2845e9e@wanadoo.fr>
Date: Fri, 27 Jun 2025 00:06:34 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Sean Christopherson <seanjc@...gle.com>
Cc: linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
 Arnaldo Carvalho de Melo <acme@...hat.com>, Kees Cook <kees@...nel.org>,
 Shuah Khan <shuah@...nel.org>
Subject: Re: [PATCH] selftests: harness: Rework is_signed_type() to avoid
 collision with overflow.h

On 25/06/2025 at 08:19, Sean Christopherson wrote:
> Rename is_signed_type() to is_signed_var() to avoid colliding with a macro
> of the same name defined by linux/overflow.h.  Note, overflow.h's version
> takes a type as the input, whereas the harness's version takes a variable!
> 
> This fixes warnings (and presumably potential test failures) in tests
> that utilize the selftests harness and happen to (indirectly) include
> overflow.h.
> 
>   In file included from tools/include/linux/bits.h:34,
>                    from tools/include/linux/bitops.h:14,
>                    from tools/include/linux/hashtable.h:13,
>                    from include/kvm_util.h:11,
>                    from x86/userspace_msr_exit_test.c:11:
>   tools/include/linux/overflow.h:31:9: error: "is_signed_type" redefined [-Werror]
>      31 | #define is_signed_type(type)       (((type)(-1)) < (type)1)
>         |         ^~~~~~~~~~~~~~
>   In file included from include/kvm_test_harness.h:11,
>                    from x86/userspace_msr_exit_test.c:9:
>   ../kselftest_harness.h:754:9: note: this is the location of the previous definition
>     754 | #define is_signed_type(var)       (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
>         |         ^~~~~~~~~~~~~~
> 
> Opportunistically use is_signed_type() to implement is_signed_var() so
> that the relationship and differences are obvious.
> 
> Fixes: fc92099902fb ("tools headers: Synchronize linux/bits.h with the kernel sources")
> Cc: Vincent Mailhol <mailhol.vincent@...adoo.fr>
> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
> 
> This is probably compile-tested only, I don't think any of the KVM selftests
> utilize the harness's EXPECT macros.
> 
>  tools/testing/selftests/kselftest_harness.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 2925e47db995..f3e7a46345db 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -56,6 +56,7 @@
>  #include <asm/types.h>
>  #include <ctype.h>
>  #include <errno.h>
> +#include <linux/overflow.h>
>  #include <linux/unistd.h>
>  #include <poll.h>
>  #include <stdbool.h>
> @@ -751,7 +752,7 @@
>  	for (; _metadata->trigger; _metadata->trigger = \
>  			__bail(_assert, _metadata))
>  
> -#define is_signed_type(var)       (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
> +#define is_signed_var(var)	is_signed_type(__typeof__(var))

It would potentially make sense to add the is_signed_var() to overflow.h. I see
a couple places doing some is_signed_type(typeof(x)):

  $ git grep "is_signed_type(typeof" | wc -l
  5

And I can imagine this number growing in the future.

But this is by no way a reason strong enough to block this patch. It is just a
random idea I am sharing here.

So that said:

Reviewed-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>

>  #define __EXPECT(_expected, _expected_str, _seen, _seen_str, _t, _assert) do { \
>  	/* Avoid multiple evaluation of the cases */ \
> @@ -759,7 +760,7 @@
>  	__typeof__(_seen) __seen = (_seen); \
>  	if (!(__exp _t __seen)) { \
>  		/* Report with actual signedness to avoid weird output. */ \
> -		switch (is_signed_type(__exp) * 2 + is_signed_type(__seen)) { \
> +		switch (is_signed_var(__exp) * 2 + is_signed_var(__seen)) { \
>  		case 0: { \
>  			uintmax_t __exp_print = (uintmax_t)__exp; \
>  			uintmax_t __seen_print = (uintmax_t)__seen; \
> 
> base-commit: 78f4e737a53e1163ded2687a922fce138aee73f5


Yours sincerely,
Vincent Mailhol