[PATCH 0/8] kselftest/arm64: various compilation fixes

[PATCH 0/8] kselftest/arm64: various compilation fixes

[Andre Przywara]

This fixes several smaller issues I faced when compiling the arm64
kselftests on my machine.
Patch 1 avoids a warning about the double definition of GNU_SOURCE,
for the arm64/signal tests. Patch 2 fixes a typo, where the f8dp2 hwcap
feature test was looking at the f8dp*4* cpuinfo name. Patch 3 adjusts
the output of the MTE tests when MTE is not available, so that tools
parsing the TAP output don't get confused and report errors.
The remaining patches are about wrong printf format specifiers. I grouped
them by type of error, in patch 4-8.

Please have a look!

Cheers,
Andre

Andre Przywara (8):
  kselftest/arm64: signal: drop now redundant GNU_SOURCE definition
  kselftest/arm64: hwcap: fix f8dp2 cpuinfo name
  kselftest/arm64: mte: use proper SKIP syntax
  kselftest/arm64: mte: use string literal for printf-style functions
  kselftest/arm64: mte: fix printf type warning about mask
  kselftest/arm64: mte: fix printf type warnings about __u64
  kselftest/arm64: mte: fix printf type warnings about pointers
  kselftest/arm64: mte: fix printf type warnings about longs

 tools/testing/selftests/arm64/abi/hwcap.c         |  2 +-
 .../selftests/arm64/mte/check_buffer_fill.c       |  4 ++--
 tools/testing/selftests/arm64/mte/check_prctl.c   |  4 ++--
 .../selftests/arm64/mte/check_tags_inclusion.c    |  4 ++--
 .../testing/selftests/arm64/mte/mte_common_util.c | 15 +++++++--------
 .../testing/selftests/arm64/mte/mte_common_util.h |  6 +++---
 tools/testing/selftests/arm64/signal/Makefile     |  2 +-
 7 files changed, 18 insertions(+), 19 deletions(-)

-- 
2.25.1

[PATCH 1/8] kselftest/arm64: signal: drop now redundant GNU_SOURCE definition

[Andre Przywara]

The definition of GNU_SOURCE was recently centralised in an upper layer
kselftest Makefile, so the definition in the arm64 signal tests Makefile
is no longer needed. To make things worse, since both definitions are
not strictly identical, the compiler warns about it:
<command-line>: warning: "_GNU_SOURCE" redefined
<command-line>: note: this is the location of the previous definition

Drop the definition in the arm64/signal Makefile.

Fixes: cc937dad85ae ("selftests: centralize -D_GNU_SOURCE= to CFLAGS in lib.mk")
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/testing/selftests/arm64/signal/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/arm64/signal/Makefile b/tools/testing/selftests/arm64/signal/Makefile
index 8f5febaf1a9a2..37c8207b99cfc 100644
--- a/tools/testing/selftests/arm64/signal/Makefile
+++ b/tools/testing/selftests/arm64/signal/Makefile
@@ -2,7 +2,7 @@
 # Copyright (C) 2019 ARM Limited
 
 # Additional include paths needed by kselftest.h and local headers
-CFLAGS += -D_GNU_SOURCE -std=gnu99 -I.
+CFLAGS += -std=gnu99 -I.
 
 SRCS := $(filter-out testcases/testcases.c,$(wildcard testcases/*.c))
 PROGS := $(patsubst %.c,%,$(SRCS))
-- 
2.25.1

[PATCH 2/8] kselftest/arm64: hwcap: fix f8dp2 cpuinfo name

[Andre Przywara]

The F8DP2 DPISA extension has a separate cpuinfo field, named
accordingly.
Change the erroneously placed name of "f8dp4" to "f8dp2".

Fixes: 44d10c27bd75 ("kselftest/arm64: Add 2023 DPISA hwcap test coverage")
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/testing/selftests/arm64/abi/hwcap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/arm64/abi/hwcap.c b/tools/testing/selftests/arm64/abi/hwcap.c
index d8909b2b535a0..41c245478e412 100644
--- a/tools/testing/selftests/arm64/abi/hwcap.c
+++ b/tools/testing/selftests/arm64/abi/hwcap.c
@@ -484,7 +484,7 @@ static const struct hwcap_data {
 		.name = "F8DP2",
 		.at_hwcap = AT_HWCAP2,
 		.hwcap_bit = HWCAP2_F8DP2,
-		.cpuinfo = "f8dp4",
+		.cpuinfo = "f8dp2",
 		.sigill_fn = f8dp2_sigill,
 	},
 	{
-- 
2.25.1

[PATCH 3/8] kselftest/arm64: mte: use proper SKIP syntax

[Andre Przywara]

If MTE is not available on a system, we detect this early and skip all
the MTE selftests. However this happens before we print the TAP plan, so
tools parsing the TAP output get confused and report an error.

Use the existing ksft_exit_skip() function to handle this, which uses a
dummy plan to work with tools expecting proper TAP syntax, as described
in the TAP specification.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/testing/selftests/arm64/mte/mte_common_util.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/arm64/mte/mte_common_util.c b/tools/testing/selftests/arm64/mte/mte_common_util.c
index 00ffd34c66d30..69e4a67853c40 100644
--- a/tools/testing/selftests/arm64/mte/mte_common_util.c
+++ b/tools/testing/selftests/arm64/mte/mte_common_util.c
@@ -319,10 +319,9 @@ int mte_default_setup(void)
 	unsigned long en = 0;
 	int ret;
 
-	if (!(hwcaps2 & HWCAP2_MTE)) {
-		ksft_print_msg("SKIP: MTE features unavailable\n");
-		return KSFT_SKIP;
-	}
+	if (!(hwcaps2 & HWCAP2_MTE))
+		ksft_exit_skip("MTE features unavailable\n");
+
 	/* Get current mte mode */
 	ret = prctl(PR_GET_TAGGED_ADDR_CTRL, en, 0, 0, 0);
 	if (ret < 0) {
-- 
2.25.1

[PATCH 4/8] kselftest/arm64: mte: use string literal for printf-style functions

[Andre Przywara]

Using pointers for the format specifier strings in printf-style
functions can create potential security problems, as the number of
arguments to be parsed could vary from call to call. Most compilers
consequently warn about those:
"format not a string literal and no format arguments [-Wformat-security]"

If we only want to print a constant string, we can just use a fixed "%s"
format instead, and pass the string as an argument.

Fixes: e9b60476bea0 ("kselftest/arm64: Add utilities and a test to validate mte memory")
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/testing/selftests/arm64/mte/mte_common_util.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/arm64/mte/mte_common_util.h b/tools/testing/selftests/arm64/mte/mte_common_util.h
index 2d3e71724e55c..a0017a303beb2 100644
--- a/tools/testing/selftests/arm64/mte/mte_common_util.h
+++ b/tools/testing/selftests/arm64/mte/mte_common_util.h
@@ -77,13 +77,13 @@ static inline void evaluate_test(int err, const char *msg)
 {
 	switch (err) {
 	case KSFT_PASS:
-		ksft_test_result_pass(msg);
+		ksft_test_result_pass("%s", msg);
 		break;
 	case KSFT_FAIL:
-		ksft_test_result_fail(msg);
+		ksft_test_result_fail("%s", msg);
 		break;
 	case KSFT_SKIP:
-		ksft_test_result_skip(msg);
+		ksft_test_result_skip("%s", msg);
 		break;
 	default:
 		ksft_test_result_error("Unknown return code %d from %s",
-- 
2.25.1

[PATCH 5/8] kselftest/arm64: mte: fix printf type warning about mask

[Andre Przywara]

When masking the return value of a prctl, which is clearly an "int", we
use a uapi header provided mask, which is defined using an "UL" modifier,
so the whole expression is promoted to a long. This upsets the compiler's
printf type checker, because we use "%x" in the format string.

While we could simply upgrade this to a "%lx", it sounds wrong to
promote the "ret" variable, that is clearly an int.
Downcast the mask instead, to keep the type correct.

Fixes: e2d9642a5a51 ("kselftest/arm64: Add simple test for MTE prctl")
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/testing/selftests/arm64/mte/check_prctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/arm64/mte/check_prctl.c b/tools/testing/selftests/arm64/mte/check_prctl.c
index f139a33a43ef4..a16d7005affdf 100644
--- a/tools/testing/selftests/arm64/mte/check_prctl.c
+++ b/tools/testing/selftests/arm64/mte/check_prctl.c
@@ -81,11 +81,11 @@ void set_mode_test(const char *name, int hwcap2, int mask)
 		return;
 	}
 
-	if ((ret & PR_MTE_TCF_MASK) == mask) {
+	if ((ret & (int)PR_MTE_TCF_MASK) == mask) {
 		ksft_test_result_pass("%s\n", name);
 	} else {
 		ksft_print_msg("Got %x, expected %x\n",
-			       (ret & PR_MTE_TCF_MASK), mask);
+			       (ret & (int)PR_MTE_TCF_MASK), mask);
 		ksft_test_result_fail("%s\n", name);
 	}
 }
-- 
2.25.1

[PATCH 6/8] kselftest/arm64: mte: fix printf type warnings about __u64

[Andre Przywara]

When printing the signal context's PC, we use a "%lx" format specifier,
which matches the common userland (glibc's) definition of uint64_t as an
"unsigned long". However the structure in question is defined in a
kernel uapi header, which uses a self defined __u64 type, and the arm64
kernel headers define this using "int-ll64.h", so it becomes an
"unsigned long long". This mismatch leads to the usual compiler warning.

The common fix would be to use "PRIx64", but because this is defined by
the userland's toolchain libc headers, it wouldn't match as well. Since
we know the exact type of __u64, just use "%llx" here instead, to silence
this warning.

This also fixes a more severe typo: "$lx" is not a valid format
specifier.

Fixes: 191e678bdc9b ("kselftest/arm64: Log unexpected asynchronous MTE faults")
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/testing/selftests/arm64/mte/mte_common_util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/arm64/mte/mte_common_util.c b/tools/testing/selftests/arm64/mte/mte_common_util.c
index 69e4a67853c40..9380edca29c7d 100644
--- a/tools/testing/selftests/arm64/mte/mte_common_util.c
+++ b/tools/testing/selftests/arm64/mte/mte_common_util.c
@@ -38,7 +38,7 @@ void mte_default_handler(int signum, siginfo_t *si, void *uc)
 			if (cur_mte_cxt.trig_si_code == si->si_code)
 				cur_mte_cxt.fault_valid = true;
 			else
-				ksft_print_msg("Got unexpected SEGV_MTEAERR at pc=$lx, fault addr=%lx\n",
+				ksft_print_msg("Got unexpected SEGV_MTEAERR at pc=%llx, fault addr=%lx\n",
 					       ((ucontext_t *)uc)->uc_mcontext.pc,
 					       addr);
 			return;
@@ -64,7 +64,7 @@ void mte_default_handler(int signum, siginfo_t *si, void *uc)
 			exit(1);
 		}
 	} else if (signum == SIGBUS) {
-		ksft_print_msg("INFO: SIGBUS signal at pc=%lx, fault addr=%lx, si_code=%lx\n",
+		ksft_print_msg("INFO: SIGBUS signal at pc=%llx, fault addr=%lx, si_code=%x\n",
 				((ucontext_t *)uc)->uc_mcontext.pc, addr, si->si_code);
 		if ((cur_mte_cxt.trig_range >= 0 &&
 		     addr >= MT_CLEAR_TAG(cur_mte_cxt.trig_addr) &&
-- 
2.25.1

[PATCH 7/8] kselftest/arm64: mte: fix printf type warnings about pointers

[Andre Przywara]

When printing the value of a pointer, we should not use an integer
format specifier, but the dedicated "%p" instead.

Fixes: e9b60476bea0 ("kselftest/arm64: Add utilities and a test to validate mte memory")
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/testing/selftests/arm64/mte/check_buffer_fill.c | 4 ++--
 tools/testing/selftests/arm64/mte/mte_common_util.c   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/arm64/mte/check_buffer_fill.c b/tools/testing/selftests/arm64/mte/check_buffer_fill.c
index 1dbbbd47dd501..2ee7f114d7fa8 100644
--- a/tools/testing/selftests/arm64/mte/check_buffer_fill.c
+++ b/tools/testing/selftests/arm64/mte/check_buffer_fill.c
@@ -91,7 +91,7 @@ static int check_buffer_underflow_by_byte(int mem_type, int mode,
 		for (j = 0; j < sizes[i]; j++) {
 			if (ptr[j] != '1') {
 				err = true;
-				ksft_print_msg("Buffer is not filled at index:%d of ptr:0x%lx\n",
+				ksft_print_msg("Buffer is not filled at index:%d of ptr:0x%p\n",
 						j, ptr);
 				break;
 			}
@@ -189,7 +189,7 @@ static int check_buffer_overflow_by_byte(int mem_type, int mode,
 		for (j = 0; j < sizes[i]; j++) {
 			if (ptr[j] != '1') {
 				err = true;
-				ksft_print_msg("Buffer is not filled at index:%d of ptr:0x%lx\n",
+				ksft_print_msg("Buffer is not filled at index:%d of ptr:0x%p\n",
 						j, ptr);
 				break;
 			}
diff --git a/tools/testing/selftests/arm64/mte/mte_common_util.c b/tools/testing/selftests/arm64/mte/mte_common_util.c
index 9380edca29c7d..17fbe5cfe4724 100644
--- a/tools/testing/selftests/arm64/mte/mte_common_util.c
+++ b/tools/testing/selftests/arm64/mte/mte_common_util.c
@@ -100,7 +100,7 @@ void *mte_insert_tags(void *ptr, size_t size)
 	int align_size;
 
 	if (!ptr || (unsigned long)(ptr) & MT_ALIGN_GRANULE) {
-		ksft_print_msg("FAIL: Addr=%lx: invalid\n", ptr);
+		ksft_print_msg("FAIL: Addr=%p: invalid\n", ptr);
 		return NULL;
 	}
 	align_size = MT_ALIGN_UP(size);
@@ -112,7 +112,7 @@ void *mte_insert_tags(void *ptr, size_t size)
 void mte_clear_tags(void *ptr, size_t size)
 {
 	if (!ptr || (unsigned long)(ptr) & MT_ALIGN_GRANULE) {
-		ksft_print_msg("FAIL: Addr=%lx: invalid\n", ptr);
+		ksft_print_msg("FAIL: Addr=%p: invalid\n", ptr);
 		return;
 	}
 	size = MT_ALIGN_UP(size);
-- 
2.25.1

[PATCH 8/8] kselftest/arm64: mte: fix printf type warnings about longs

[Andre Przywara]

When checking MTE tags, we print some diagnostic messages when the tests
fail. Some variables uses there are "longs", however we only use "%x"
for the format specifier.

Update the format specifiers to "%lx", to match the variable types they
are supposed to print.

Fixes: f3b2a26ca78d ("kselftest/arm64: Verify mte tag inclusion via prctl")
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/testing/selftests/arm64/mte/check_tags_inclusion.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/arm64/mte/check_tags_inclusion.c b/tools/testing/selftests/arm64/mte/check_tags_inclusion.c
index 2b1425b92b699..a3d1e23fe02af 100644
--- a/tools/testing/selftests/arm64/mte/check_tags_inclusion.c
+++ b/tools/testing/selftests/arm64/mte/check_tags_inclusion.c
@@ -65,7 +65,7 @@ static int check_single_included_tags(int mem_type, int mode)
 			ptr = mte_insert_tags(ptr, BUFFER_SIZE);
 			/* Check tag value */
 			if (MT_FETCH_TAG((uintptr_t)ptr) == tag) {
-				ksft_print_msg("FAIL: wrong tag = 0x%x with include mask=0x%x\n",
+				ksft_print_msg("FAIL: wrong tag = 0x%lx with include mask=0x%x\n",
 					       MT_FETCH_TAG((uintptr_t)ptr),
 					       MT_INCLUDE_VALID_TAG(tag));
 				result = KSFT_FAIL;
@@ -97,7 +97,7 @@ static int check_multiple_included_tags(int mem_type, int mode)
 			ptr = mte_insert_tags(ptr, BUFFER_SIZE);
 			/* Check tag value */
 			if (MT_FETCH_TAG((uintptr_t)ptr) < tag) {
-				ksft_print_msg("FAIL: wrong tag = 0x%x with include mask=0x%x\n",
+				ksft_print_msg("FAIL: wrong tag = 0x%lx with include mask=0x%lx\n",
 					       MT_FETCH_TAG((uintptr_t)ptr),
 					       MT_INCLUDE_VALID_TAGS(excl_mask));
 				result = KSFT_FAIL;
-- 
2.25.1

Re: [PATCH 1/8] kselftest/arm64: signal: drop now redundant GNU_SOURCE definition

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]

On Fri, Aug 16, 2024 at 04:32:44PM +0100, Andre Przywara wrote:
> The definition of GNU_SOURCE was recently centralised in an upper layer
> kselftest Makefile, so the definition in the arm64 signal tests Makefile
> is no longer needed. To make things worse, since both definitions are
> not strictly identical, the compiler warns about it:
> <command-line>: warning: "_GNU_SOURCE" redefined
> <command-line>: note: this is the location of the previous definition

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/8] kselftest/arm64: hwcap: fix f8dp2 cpuinfo name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 249 bytes --]

On Fri, Aug 16, 2024 at 04:32:45PM +0100, Andre Przywara wrote:
> The F8DP2 DPISA extension has a separate cpuinfo field, named
> accordingly.
> Change the erroneously placed name of "f8dp4" to "f8dp2".

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/8] kselftest/arm64: mte: use proper SKIP syntax

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 323 bytes --]

On Fri, Aug 16, 2024 at 04:32:46PM +0100, Andre Przywara wrote:
> If MTE is not available on a system, we detect this early and skip all
> the MTE selftests. However this happens before we print the TAP plan, so
> tools parsing the TAP output get confused and report an error.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/8] kselftest/arm64: mte: use string literal for printf-style functions

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 796 bytes --]

On Fri, Aug 16, 2024 at 04:32:47PM +0100, Andre Przywara wrote:
> Using pointers for the format specifier strings in printf-style
> functions can create potential security problems, as the number of
> arguments to be parsed could vary from call to call. Most compilers
> consequently warn about those:
> "format not a string literal and no format arguments [-Wformat-security]"
> 
> If we only want to print a constant string, we can just use a fixed "%s"
> format instead, and pass the string as an argument.
> 
> Fixes: e9b60476bea0 ("kselftest/arm64: Add utilities and a test to validate mte memory")

I'm not sure this qualifies as a fix given that all the strings we're
passing in here are trusted...  otheriwse this looks good.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/8] kselftest/arm64: mte: fix printf type warning about mask

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

On Fri, Aug 16, 2024 at 04:32:48PM +0100, Andre Przywara wrote:
> When masking the return value of a prctl, which is clearly an "int", we
> use a uapi header provided mask, which is defined using an "UL" modifier,
> so the whole expression is promoted to a long. This upsets the compiler's
> printf type checker, because we use "%x" in the format string.
> 
> While we could simply upgrade this to a "%lx", it sounds wrong to
> promote the "ret" variable, that is clearly an int.
> Downcast the mask instead, to keep the type correct.

This suggests that we've got some confusion with the UAPI, these flags
need to go through a prctl() return so they shouldn't be unsigned
long...  That said, it's UAPI so I'm not sure that's fixable.

> -	if ((ret & PR_MTE_TCF_MASK) == mask) {
> +	if ((ret & (int)PR_MTE_TCF_MASK) == mask) {
>  		ksft_test_result_pass("%s\n", name);
>  	} else {
>  		ksft_print_msg("Got %x, expected %x\n",
> -			       (ret & PR_MTE_TCF_MASK), mask);
> +			       (ret & (int)PR_MTE_TCF_MASK), mask);
>  		ksft_test_result_fail("%s\n", name);

TBH my inclination is that this is worse than letting the value be
promoted, casts (particularly casts of constants) are just obviously
suspect in a way that printf() formats aren't.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 6/8] kselftest/arm64: mte: fix printf type warnings about __u64

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 543 bytes --]

On Fri, Aug 16, 2024 at 04:32:49PM +0100, Andre Przywara wrote:
> When printing the signal context's PC, we use a "%lx" format specifier,
> which matches the common userland (glibc's) definition of uint64_t as an
> "unsigned long". However the structure in question is defined in a
> kernel uapi header, which uses a self defined __u64 type, and the arm64
> kernel headers define this using "int-ll64.h", so it becomes an
> "unsigned long long". This mismatch leads to the usual compiler warning.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 7/8] kselftest/arm64: mte: fix printf type warnings about pointers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

On Fri, Aug 16, 2024 at 04:32:50PM +0100, Andre Przywara wrote:
> When printing the value of a pointer, we should not use an integer
> format specifier, but the dedicated "%p" instead.
> 
> Fixes: e9b60476bea0 ("kselftest/arm64: Add utilities and a test to validate mte memory")

This is another one where calling it a fix seems like it's pushing it,
it's a modernisation rather than a correctness thing.  Otherwise

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 8/8] kselftest/arm64: mte: fix printf type warnings about longs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 285 bytes --]

On Fri, Aug 16, 2024 at 04:32:51PM +0100, Andre Przywara wrote:
> When checking MTE tags, we print some diagnostic messages when the tests
> fail. Some variables uses there are "longs", however we only use "%x"
> for the format specifier.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/8] kselftest/arm64: mte: fix printf type warning about mask

[Andre Przywara]

On Fri, 16 Aug 2024 17:30:14 +0100
Mark Brown <broonie@kernel.org> wrote:

Hi Mark,

thanks for having a look!

> On Fri, Aug 16, 2024 at 04:32:48PM +0100, Andre Przywara wrote:
> > When masking the return value of a prctl, which is clearly an "int", we
> > use a uapi header provided mask, which is defined using an "UL" modifier,
> > so the whole expression is promoted to a long. This upsets the compiler's
> > printf type checker, because we use "%x" in the format string.
> > 
> > While we could simply upgrade this to a "%lx", it sounds wrong to
> > promote the "ret" variable, that is clearly an int.
> > Downcast the mask instead, to keep the type correct.  
> 
> This suggests that we've got some confusion with the UAPI, these flags
> need to go through a prctl() return so they shouldn't be unsigned
> long...  That said, it's UAPI so I'm not sure that's fixable.

My thoughts, exactly. Not sure if this mask is used on some larger types
somewhere else, but it's indeed moot since it's UAPI.

> > -	if ((ret & PR_MTE_TCF_MASK) == mask) {
> > +	if ((ret & (int)PR_MTE_TCF_MASK) == mask) {
> >  		ksft_test_result_pass("%s\n", name);
> >  	} else {
> >  		ksft_print_msg("Got %x, expected %x\n",
> > -			       (ret & PR_MTE_TCF_MASK), mask);
> > +			       (ret & (int)PR_MTE_TCF_MASK), mask);
> >  		ksft_test_result_fail("%s\n", name);  
> 
> TBH my inclination is that this is worse than letting the value be
> promoted, casts (particularly casts of constants) are just obviously
> suspect in a way that printf() formats aren't.

Fair enough, I wasn't sure about this either, and indeed this down-cast of
a constant smells dodgy. So shall I just use %lx, and rely on the
promotion (so the MASK being defined as UL), or cast "ret" to long?

Cheers,
Andre

Re: [PATCH 7/8] kselftest/arm64: mte: fix printf type warnings about pointers

[Andre Przywara]

On Fri, 16 Aug 2024 17:32:39 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Fri, Aug 16, 2024 at 04:32:50PM +0100, Andre Przywara wrote:
> > When printing the value of a pointer, we should not use an integer
> > format specifier, but the dedicated "%p" instead.
> > 
> > Fixes: e9b60476bea0 ("kselftest/arm64: Add utilities and a test to validate mte memory")  
> 
> This is another one where calling it a fix seems like it's pushing it,
> it's a modernisation rather than a correctness thing.

Well, I get compiler warnings, so I thought "fix" would be adequate. But
in general this confusion between pointers and integers sounds not good,
and not using %p looks like a genuine bug to me (though it's admittedly
working fine (TM) for now).

> Otherwise
> 
> Reviewed-by: Mark Brown <broonie@kernel.org>

Thanks for that!

Cheers,
Andre

Re: [PATCH 5/8] kselftest/arm64: mte: fix printf type warning about mask

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 786 bytes --]

On Fri, Aug 16, 2024 at 05:55:48PM +0100, Andre Przywara wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Aug 16, 2024 at 04:32:48PM +0100, Andre Przywara wrote:

> > >  		ksft_print_msg("Got %x, expected %x\n",
> > > -			       (ret & PR_MTE_TCF_MASK), mask);
> > > +			       (ret & (int)PR_MTE_TCF_MASK), mask);

> > TBH my inclination is that this is worse than letting the value be
> > promoted, casts (particularly casts of constants) are just obviously
> > suspect in a way that printf() formats aren't.

> Fair enough, I wasn't sure about this either, and indeed this down-cast of
> a constant smells dodgy. So shall I just use %lx, and rely on the
> promotion (so the MASK being defined as UL), or cast "ret" to long?

My inclination would be to go with the former.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 7/8] kselftest/arm64: mte: fix printf type warnings about pointers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

On Fri, Aug 16, 2024 at 05:59:08PM +0100, Andre Przywara wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Aug 16, 2024 at 04:32:50PM +0100, Andre Przywara wrote:

> > > Fixes: e9b60476bea0 ("kselftest/arm64: Add utilities and a test to validate mte memory")  

> > This is another one where calling it a fix seems like it's pushing it,
> > it's a modernisation rather than a correctness thing.

> Well, I get compiler warnings, so I thought "fix" would be adequate. But
> in general this confusion between pointers and integers sounds not good,
> and not using %p looks like a genuine bug to me (though it's admittedly
> working fine (TM) for now).

Those compiler warnings are a relatively new thing - they won't have
been triggered when the code was written, and practically speaking on
arm64 it's more of a C language correctness thing than a meaningful
change.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (subset) [PATCH 0/8] kselftest/arm64: various compilation fixes

[Catalin Marinas]

On Fri, 16 Aug 2024 16:32:43 +0100, Andre Przywara wrote:
> This fixes several smaller issues I faced when compiling the arm64
> kselftests on my machine.
> Patch 1 avoids a warning about the double definition of GNU_SOURCE,
> for the arm64/signal tests. Patch 2 fixes a typo, where the f8dp2 hwcap
> feature test was looking at the f8dp*4* cpuinfo name. Patch 3 adjusts
> the output of the MTE tests when MTE is not available, so that tools
> parsing the TAP output don't get confused and report errors.
> The remaining patches are about wrong printf format specifiers. I grouped
> them by type of error, in patch 4-8.
> 
> [...]

Applied to arm64 (for-next/kselftest), thanks!

I skiped patch 5 since Mark wanted it handled differently. Andre, would
you mind respinning that patch?

[1/8] kselftest/arm64: signal: drop now redundant GNU_SOURCE definition
      https://git.kernel.org/arm64/c/a2aa5dcc6393
[2/8] kselftest/arm64: hwcap: fix f8dp2 cpuinfo name
      https://git.kernel.org/arm64/c/b0d80dbc378d
[3/8] kselftest/arm64: mte: use proper SKIP syntax
      https://git.kernel.org/arm64/c/bf52ca5912c0
[4/8] kselftest/arm64: mte: use string literal for printf-style functions
      https://git.kernel.org/arm64/c/0f995f22a03f
[6/8] kselftest/arm64: mte: fix printf type warnings about __u64
      https://git.kernel.org/arm64/c/7e893dc81de3
[7/8] kselftest/arm64: mte: fix printf type warnings about pointers
      https://git.kernel.org/arm64/c/4716f719202e
[8/8] kselftest/arm64: mte: fix printf type warnings about longs
      https://git.kernel.org/arm64/c/96dddb7b9406

-- 
Catalin