kselftest/arm64: pac: Fix skipping of tests on systems without PAC

PATCH In review Mark Brown 3 participants 6 messages Aug 19, 2021 – Aug 20, 2021

Subsystems (2)

Kselftest arm64

lore

Share & tools

lore

Mark Brown

author

2021-08-19 16:57 UTC #0 lore raw

PATCH 1/1 kselftest/arm64: pac: Fix skipping of tests on systems without PAC

The PAC tests check to see if the system supports the relevant PAC features but instead of skipping the tests if they can't be executed they fail the tests which makes things look like they're not working when they are.

Signed-off-by Mark Brown <[email protected]>

---
 tools/testing/selftests/arm64/pauth/pac.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

tools/testing/selftests/arm64/pauth/pac.c +6 -4

index 592fe538506e..b743daa772f5 100644
@@ -25,13 +25,15 @@
 do { \
 	unsigned long hwcaps = getauxval(AT_HWCAP); \
 	/* data key instructions are not in NOP space. This prevents a SIGILL */ \
-	ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
+	if (!(hwcaps & HWCAP_PACA))					\
+		SKIP(return, "PAUTH not enabled"); \
 } while (0)
 #define ASSERT_GENERIC_PAUTH_ENABLED() \
 do { \
 	unsigned long hwcaps = getauxval(AT_HWCAP); \
 	/* generic key instructions are not in NOP space. This prevents a SIGILL */ \
-	ASSERT_NE(0, hwcaps & HWCAP_PACG) TH_LOG("Generic PAUTH not enabled"); \
+	if (!(hwcaps & HWCAP_PACG)) \
+		SKIP(return, "Generic PAUTH not enabled");	\
 } while (0)
 
 void sign_specific(struct signatures *sign, size_t val)

@@ -256,7 +258,7 @@ TEST(single_thread_different_keys)
 	unsigned long hwcaps = getauxval(AT_HWCAP);
 
 	/* generic and data key instructions are not in NOP space. This prevents a SIGILL */
-	ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled");
+	ASSERT_PAUTH_ENABLED();
 	if (!(hwcaps & HWCAP_PACG)) {
 		TH_LOG("WARNING: Generic PAUTH not enabled. Skipping generic key checks");
 		nkeys = NKEYS - 1;

@@ -299,7 +301,7 @@ TEST(exec_changed_keys)
 	unsigned long hwcaps = getauxval(AT_HWCAP);
 
 	/* generic and data key instructions are not in NOP space. This prevents a SIGILL */
-	ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled");
+	ASSERT_PAUTH_ENABLED();
 	if (!(hwcaps & HWCAP_PACG)) {
 		TH_LOG("WARNING: Generic PAUTH not enabled. Skipping generic key checks");
 		nkeys = NKEYS - 1;

Signature · 1 line

2.20.1

Amit Kachhap

2021-08-20 07:09 UTC #1 lore raw

Quoted 14 lines · Mark Brown

The PAC tests check to see if the system supports the relevant PAC features but instead of skipping the tests if they can't be executed they fail the tests which makes things look like they're not working when they are.
Signed-off-by Mark Brown <[email protected]>
---
  tools/testing/selftests/arm64/pauth/pac.c | 10 ++++++----
  1 file changed, 6 insertions(+), 4 deletions(-)
tools/testing/selftests/arm64/pauth/pac.c +0 -0
index 592fe538506e..b743daa772f5 100644

tools/testing/selftests/arm64/pauth/pac.c line 31

Quoted 8 lines

tools/testing/selftests/arm64/pauth/pac.c +2 -1

@@ -25,13 +25,15 @@
  do { \
  	unsigned long hwcaps = getauxval(AT_HWCAP); \
  	/* data key instructions are not in NOP space. This prevents a SIGILL */ \
-	ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
+	if (!(hwcaps & HWCAP_PACA))					\
+		SKIP(return, "PAUTH not enabled"); \
  } while (0)
  #define ASSERT_GENERIC_PAUTH_ENABLED() \

May be ASSERT_GENERIC_PAUTH_ENABLED can be replaced with something like VERIFY_GENERIC_PAUTH_ENABLED

or can be modified like below and instead of failing it skips with a message

- ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
+ ASSERT_NE(0, hwcaps & HWCAP_PACA) SKIP(return, "PAUTH not
enabled"); \

Quoted 28 lines

tools/testing/selftests/arm64/pauth/pac.c +4 -3

  do { \
  	unsigned long hwcaps = getauxval(AT_HWCAP); \
  	/* generic key instructions are not in NOP space. This prevents a SIGILL */ \
-	ASSERT_NE(0, hwcaps & HWCAP_PACG) TH_LOG("Generic PAUTH not enabled"); \
+	if (!(hwcaps & HWCAP_PACG)) \
+		SKIP(return, "Generic PAUTH not enabled");	\
  } while (0)
 
  void sign_specific(struct signatures *sign, size_t val)

@@ -256,7 +258,7 @@ TEST(single_thread_different_keys)
  	unsigned long hwcaps = getauxval(AT_HWCAP);
 
  	/* generic and data key instructions are not in NOP space. This prevents a SIGILL */
-	ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled");
+	ASSERT_PAUTH_ENABLED();
  	if (!(hwcaps & HWCAP_PACG)) {
  		TH_LOG("WARNING: Generic PAUTH not enabled. Skipping generic key checks");
  		nkeys = NKEYS - 1;

@@ -299,7 +301,7 @@ TEST(exec_changed_keys)
  	unsigned long hwcaps = getauxval(AT_HWCAP);
 
  	/* generic and data key instructions are not in NOP space. This prevents a SIGILL */
-	ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled");
+	ASSERT_PAUTH_ENABLED();
  	if (!(hwcaps & HWCAP_PACG)) {
  		TH_LOG("WARNING: Generic PAUTH not enabled. Skipping generic key checks");
  		nkeys = NKEYS - 1;

Mark Brown

author

2021-08-20 10:55 UTC #2 lore raw

Quoted 10 lines · Amit Kachhap

tools/testing/selftests/arm64/pauth/pac.c line 31
Quoted 5 lines · Mark Brown
tools/testing/selftests/arm64/pauth/pac.c +2 -1
-	ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
+	if (!(hwcaps & HWCAP_PACA))					\
+		SKIP(return, "PAUTH not enabled"); \
  } while (0)
  #define ASSERT_GENERIC_PAUTH_ENABLED() \
May be ASSERT_GENERIC_PAUTH_ENABLED can be replaced with something like VERIFY_GENERIC_PAUTH_ENABLED

I thought briefly about bikeshedding the name but didn't come up with anything that was sufficiently better/clearer.

Quoted 5 lines

or can be modified like below and instead of failing it skips with a message
  -	ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
  +	ASSERT_NE(0, hwcaps & HWCAP_PACA) SKIP(return, "PAUTH not enabled");

That's what the patch does?

Amit Kachhap

2021-08-20 11:35 UTC #3 lore raw

Quoted 15 lines · Mark Brown

Quoted 10 lines · Amit Kachhap
tools/testing/selftests/arm64/pauth/pac.c line 31
Quoted 5 lines · Mark Brown
tools/testing/selftests/arm64/pauth/pac.c +2 -1
-	ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
+	if (!(hwcaps & HWCAP_PACA))					\
+		SKIP(return, "PAUTH not enabled"); \
   } while (0)
   #define ASSERT_GENERIC_PAUTH_ENABLED() \
May be ASSERT_GENERIC_PAUTH_ENABLED can be replaced with something like VERIFY_GENERIC_PAUTH_ENABLED
I thought briefly about bikeshedding the name but didn't come up with anything that was sufficiently better/clearer.

Quoted 7 lines

Quoted 5 lines
or can be modified like below and instead of failing it skips with a message
   -	ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
   +	ASSERT_NE(0, hwcaps & HWCAP_PACA) SKIP(return, "PAUTH not enabled");
That's what the patch does?

Agree, I saw few other testcases where ASSERT is used along with SKIP. (tools/testing/selftests/core/close_range_test.c) so this way ASSERT_GENERIC_* macro will be clear. There will be just an extra log like "hwcaps & HWCAP_PACA = 0" in this case.

Probably your way is consistent as other tests in arm64 also just skips due to HWCAP mismatch.

Quoted 1 line

Mark Brown

author

2021-08-20 12:00 UTC #4 lore raw

Quoted 6 lines · Amit Kachhap

Quoted 4 lines · Mark Brown

Quoted 2 lines · Amit Kachhap

   -	ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
   +	ASSERT_NE(0, hwcaps & HWCAP_PACA) SKIP(return, "PAUTH not enabled");

Quoted 6 lines

Quoted 1 line
That's what the patch does?
Agree, I saw few other testcases where ASSERT is used along with SKIP. (tools/testing/selftests/core/close_range_test.c) so this way ASSERT_GENERIC_* macro will be clear. There will be just an extra log like "hwcaps & HWCAP_PACA = 0" in this case.

I'm not 100% sure I follow what you're saying here? I don't see a log message saying "hwcaps & HWCAP_PACA = 0" on non-PAC systems, or is that what you're saying you want to see?

Quoted 2 lines

Probably your way is consistent as other tests in arm64 also just skips due to HWCAP mismatch.

Right, we can't usefully run tests for features not present in the system - this sort of thing is the intent of the kselftest skip feature.

Catalin Marinas

maintainer

2021-08-20 16:57 UTC #5 lore raw

In reply to Mark Brown

Quoted 3 lines · Mark Brown

The PAC tests check to see if the system supports the relevant PAC features but instead of skipping the tests if they can't be executed they fail the tests which makes things look like they're not working when they are.

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

[1/1] kselftest/arm64: pac: Fix skipping of tests on systems without PAC

      https://git.kernel.org/arm64/c/0c69bd2ca6ee

Signature · 1 line

Catalin