Hi Cristian,

On 1/26/22 10:27 AM, Cristian Marussi wrote:

When a vDSO symbol is not found, all the testcases in vdso_test_abi usually report a SKIP, which, in turn, is reported back to Kselftest as a PASS.

Testcase vdso_test_time, instead, reporting a SKIP, causes the whole set of tests within vdso_test_abi to be considered FAIL when symbol is not found.

Fix it reporting a PASS when vdso_test_time cannot find the vdso symbol.

Cc: Vincenzo Frascino vincenzo.frascino@arm.com Signed-off-by: Cristian Marussi cristian.marussi@arm.com

---

Seen as a failure on both a JUNO and a Dragonboard on both recent and old kernels/testruns:

root@deb-buster-arm64:~# /opt/ksft/vDSO/vdso_test_abi [vDSO kselftest] VDSO_VERSION: LINUX_2.6.39 The time is 1637922136.675304 The time is 1637922136.675361000 The resolution is 0 1 clock_id: CLOCK_REALTIME [PASS] The time is 1927.760604900 The resolution is 0 1 clock_id: CLOCK_BOOTTIME [PASS] The time is 1637922136.675649700 The resolution is 0 1 clock_id: CLOCK_TAI [PASS] The time is 1637922136.672000000 The resolution is 0 4000000 clock_id: CLOCK_REALTIME_COARSE [PASS] The time is 1927.761005600 The resolution is 0 1 clock_id: CLOCK_MONOTONIC [PASS] The time is 1927.761132780 The resolution is 0 1 clock_id: CLOCK_MONOTONIC_RAW [PASS] The time is 1927.757093740 The resolution is 0 4000000 clock_id: CLOCK_MONOTONIC_COARSE [PASS] Could not find __kernel_time <<< This caused a FAIL as a whole root@deb-buster-arm64:~# echo $? 1

e.g.: https://lkft.validation.linaro.org/scheduler/job/2192570#L27778

tools/testing/selftests/vDSO/vdso_test_abi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/vDSO/vdso_test_abi.c b/tools/testing/selftests/vDSO/vdso_test_abi.c index 3d603f1394af..7dcc66d1cecf 100644 --- a/tools/testing/selftests/vDSO/vdso_test_abi.c +++ b/tools/testing/selftests/vDSO/vdso_test_abi.c @@ -90,8 +90,9 @@ static int vdso_test_time(void) (vdso_time_t)vdso_sym(version, name[2]); if (!vdso_time) {

• /* Skip if symbol not found: consider skipped tests as passed */
  

  printf("Could not find %s\n", name[2]);

• return KSFT_SKIP;
  

• return KSFT_PASS;
  

My preference would be to keep "KSFT_SKIP" here and verify separately the return status of each test. This would maintain compliance with the kselftest API. Could you please test the patch in-reply-to this one (will be sent shortly) and let me know if it works for you?

If it does feel free to fold it in the next version of your series with your "Tested-by:" otherwise let me know.

Thanks!

} long ret = vdso_time(NULL);

On 1/26/22 5:26 AM, Vincenzo Frascino wrote:

vdso_test_abi contains a batch of tests that verify the validity of the vDSO ABI.

When a vDSO symbol is not found the relevant test is skipped reporting KSFT_SKIP. All the tests return values are then added in a single variable which is checked to verify failures. This approach can have side effects which result in reporting the wrong kselftest exit status.

Fix vdso_test_abi verifying the return code of each test separately.

Cc: Shuah Khan shuah@kernel.org Cc: Andy Lutomirski luto@kernel.org Cc: Thomas Gleixner tglx@linutronix.de Reported-by: Cristian Marussi cristian.marussi@arm.com Signed-off-by: Vincenzo Frascino vincenzo.frascino@arm.com

---

tools/testing/selftests/vDSO/vdso_test_abi.c | 27 +++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/vDSO/vdso_test_abi.c b/tools/testing/selftests/vDSO/vdso_test_abi.c index 3d603f1394af..3a4efb91b9b2 100644 --- a/tools/testing/selftests/vDSO/vdso_test_abi.c +++ b/tools/testing/selftests/vDSO/vdso_test_abi.c @@ -184,10 +184,12 @@ static inline int vdso_test_clock(clockid_t clock_id) return ret0; } +#define VDSO_TESTS_MAX 9

• int main(int argc, char **argv) { unsigned long sysinfo_ehdr = getauxval(AT_SYSINFO_EHDR);

• int ret;

• int ret[VDSO_TESTS_MAX] = {0};

if (!sysinfo_ehdr) { printf("AT_SYSINFO_EHDR is not present!\n"); @@ -201,44 +203,45 @@ int main(int argc, char **argv) vdso_init_from_sysinfo_ehdr(getauxval(AT_SYSINFO_EHDR));

• ret = vdso_test_gettimeofday();

• ret[0] = vdso_test_gettimeofday();

#if _POSIX_TIMERS > 0 #ifdef CLOCK_REALTIME

• ret += vdso_test_clock(CLOCK_REALTIME);

• ret[1] = vdso_test_clock(CLOCK_REALTIME); #endif

#ifdef CLOCK_BOOTTIME

• ret += vdso_test_clock(CLOCK_BOOTTIME);

• ret[2] = vdso_test_clock(CLOCK_BOOTTIME); #endif

#ifdef CLOCK_TAI

• ret += vdso_test_clock(CLOCK_TAI);

• ret[3] = vdso_test_clock(CLOCK_TAI); #endif

#ifdef CLOCK_REALTIME_COARSE

• ret += vdso_test_clock(CLOCK_REALTIME_COARSE);

• ret[4] = vdso_test_clock(CLOCK_REALTIME_COARSE); #endif

#ifdef CLOCK_MONOTONIC

• ret += vdso_test_clock(CLOCK_MONOTONIC);

• ret[5] = vdso_test_clock(CLOCK_MONOTONIC); #endif

#ifdef CLOCK_MONOTONIC_RAW

• ret += vdso_test_clock(CLOCK_MONOTONIC_RAW);

• ret[6] = vdso_test_clock(CLOCK_MONOTONIC_RAW); #endif

#ifdef CLOCK_MONOTONIC_COARSE

• ret += vdso_test_clock(CLOCK_MONOTONIC_COARSE);

• ret[7] = vdso_test_clock(CLOCK_MONOTONIC_COARSE); #endif

#endif

• ret += vdso_test_time();

• ret[8] = vdso_test_time();

• if (ret > 0)

• return KSFT_FAIL;
  

• for (int i = 0; i < VDSO_TESTS_MAX; i++)

• if (ret[i] == KSFT_FAIL)
  

• 	return KSFT_FAIL;
  

return KSFT_PASS; }

You can use the ksft_* counts interfaces for this instead of adding counts here. ksft_test_result_*() can be used to increment the right result counters and then print counts at the end.

Either if there is a failure in any of the tests it will be fail with clear indication on which tests failed. vdso_test_clock() test for example is reporting false positives by overriding the Skip return with a pass.

thanks, -- Shuah

On 1/26/22 3:27 AM, Cristian Marussi wrote:

When a vDSO symbol is not found, all the testcases in vdso_test_abi usually report a SKIP, which, in turn, is reported back to Kselftest as a PASS.

Testcase vdso_test_time, instead, reporting a SKIP, causes the whole set of tests within vdso_test_abi to be considered FAIL when symbol is not found.

Fix it reporting a PASS when vdso_test_time cannot find the vdso symbol.

Cc: Vincenzo Frascino vincenzo.frascino@arm.com Signed-off-by: Cristian Marussi cristian.marussi@arm.com

---

Seen as a failure on both a JUNO and a Dragonboard on both recent and old kernels/testruns:

root@deb-buster-arm64:~# /opt/ksft/vDSO/vdso_test_abi [vDSO kselftest] VDSO_VERSION: LINUX_2.6.39 The time is 1637922136.675304 The time is 1637922136.675361000 The resolution is 0 1 clock_id: CLOCK_REALTIME [PASS] The time is 1927.760604900 The resolution is 0 1 clock_id: CLOCK_BOOTTIME [PASS] The time is 1637922136.675649700 The resolution is 0 1 clock_id: CLOCK_TAI [PASS] The time is 1637922136.672000000 The resolution is 0 4000000 clock_id: CLOCK_REALTIME_COARSE [PASS] The time is 1927.761005600 The resolution is 0 1 clock_id: CLOCK_MONOTONIC [PASS] The time is 1927.761132780 The resolution is 0 1 clock_id: CLOCK_MONOTONIC_RAW [PASS] The time is 1927.757093740 The resolution is 0 4000000 clock_id: CLOCK_MONOTONIC_COARSE [PASS] Could not find __kernel_time <<< This caused a FAIL as a whole root@deb-buster-arm64:~# echo $? 1

e.g.: https://lkft.validation.linaro.org/scheduler/job/2192570#L27778

tools/testing/selftests/vDSO/vdso_test_abi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/vDSO/vdso_test_abi.c b/tools/testing/selftests/vDSO/vdso_test_abi.c index 3d603f1394af..7dcc66d1cecf 100644 --- a/tools/testing/selftests/vDSO/vdso_test_abi.c +++ b/tools/testing/selftests/vDSO/vdso_test_abi.c @@ -90,8 +90,9 @@ static int vdso_test_time(void) (vdso_time_t)vdso_sym(version, name[2]); if (!vdso_time) {

• /* Skip if symbol not found: consider skipped tests as passed */
  

  printf("Could not find %s\n", name[2]);

• return KSFT_SKIP;
  

• return KSFT_PASS;
  

Skip is a the right option here. Pass indicates that the functionality has been tested and it passed. There is a clear message that says that the symbol isn't found

thanks, -- Shuah

On 1/26/22 3:27 AM, Cristian Marussi wrote:

In E_func() macro, on error, print also errno in order to aid debugging.

Cc: Aleksa Sarai cyphar@cyphar.com Signed-off-by: Cristian Marussi cristian.marussi@arm.com

---

tools/testing/selftests/openat2/helpers.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h index a6ea27344db2..ad5d0ba5b6ce 100644 --- a/tools/testing/selftests/openat2/helpers.h +++ b/tools/testing/selftests/openat2/helpers.h @@ -62,11 +62,12 @@ bool needs_openat2(const struct open_how *how); (similar to chroot(2)). */ #endif /* RESOLVE_IN_ROOT */ -#define E_func(func, ...) \

• do { \

• if (func(__VA_ARGS__) < 0)				\
  

• 	ksft_exit_fail_msg("%s:%d %s failed\n", \
  

• 			   __FILE__, __LINE__, #func);\
  

+#define E_func(func, ...) \

• do { \

• errno = 0;						      \
  

• if (func(__VA_ARGS__) < 0)				      \
  

• 	ksft_exit_fail_msg("%s:%d %s failed - errno:%d\n",    \
  

• 			   __FILE__, __LINE__, #func, errno); \
  

  } while (0)

#define E_asprintf(...) E_func(asprintf, __VA_ARGS__)

Looks good. Will apply to linux-kselftest rc3

thanks, -- Shuah

On 1/26/22 3:27 AM, Cristian Marussi wrote:

Add a dependency on header helpers.h to the main target; while at that add to helpers.h also a missing include for bool types.

Cc: Aleksa Sarai cyphar@cyphar.com Signed-off-by: Cristian Marussi cristian.marussi@arm.com

---

tools/testing/selftests/openat2/Makefile | 2 +- tools/testing/selftests/openat2/helpers.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/openat2/Makefile b/tools/testing/selftests/openat2/Makefile index 4b93b1417b86..843ba56d8e49 100644 --- a/tools/testing/selftests/openat2/Makefile +++ b/tools/testing/selftests/openat2/Makefile @@ -5,4 +5,4 @@ TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test include ../lib.mk -$(TEST_GEN_PROGS): helpers.c +$(TEST_GEN_PROGS): helpers.c helpers.h diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h index ad5d0ba5b6ce..7056340b9339 100644 --- a/tools/testing/selftests/openat2/helpers.h +++ b/tools/testing/selftests/openat2/helpers.h @@ -9,6 +9,7 @@ #define _GNU_SOURCE #include <stdint.h> +#include <stdbool.h> #include <errno.h> #include <linux/types.h> #include "../kselftest.h"

Thanks for the patch. Will apply for linux-kselftest fixes branch for rc3

thanks, -- Shuah

On 1/26/22 3:27 AM, Cristian Marussi wrote:

Skip testcases that fail since the requested valid flags combination is not supported by the underlying filesystem.

Cc: Aleksa Sarai cyphar@cyphar.com Signed-off-by: Cristian Marussi cristian.marussi@arm.com

---

tools/testing/selftests/openat2/openat2_test.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c index 1bddbe934204..7fb902099de4 100644 --- a/tools/testing/selftests/openat2/openat2_test.c +++ b/tools/testing/selftests/openat2/openat2_test.c @@ -259,6 +259,16 @@ void test_openat2_flags(void) unlink(path); fd = sys_openat2(AT_FDCWD, path, &test->how);

• if (fd < 0 && fd == -EOPNOTSUPP) {
  

• 	/*
  

• 	 * Skip the testcase if it failed because not supported
  

• 	 * by FS. (e.g. a valid O_TMPFILE combination on NFS)
  

• 	 */
  

• 	ksft_test_result_skip("openat2 with %s fails with %d (%s)\n",
  

• 			      test->name, fd, strerror(-fd));
  

• 	goto next;
  

• }
  

• if (test->err >= 0) failed = (fd < 0); else

@@ -303,7 +313,7 @@ void test_openat2_flags(void) else resultfn("openat2 with %s fails with %d (%s)\n", test->name, test->err, strerror(-test->err));

+next: free(fdpath); fflush(stdout); }

Thanks for the patch. Will apply to linux-kselftest fixes branc for rc3

thanks, -- Shuah