[PATCH 0/2] kselftest/exec: Convert execveat test to KTAP output

[PATCH 0/2] kselftest/exec: Convert execveat test to KTAP output

[Mark Brown]

This series converts the execveat test to generate KTAP output so it
plays a bit more nicely with automation, KTAP means that kselftest
runners can track the individual tests in the suite rather than just an
overall pass/fail for the suite as a whole.

The first patch adding a perror() equivalent for kselftest was
previously sent as part of a similar conversion for the timers tests:

   https://lore.kernel.org/linux-kselftest/8734yyfx00.ffs@tglx/T

there's probably no harm in applying it twice or possibly these should
both go via the kselftest tree - I'm not sure who usually applies timers
test changes.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
Mark Brown (2):
      kselftest: Add a ksft_perror() helper
      selftests/exec: Convert execveat test to generate KTAP output

 tools/testing/selftests/exec/execveat.c | 87 ++++++++++++++++++++-------------
 tools/testing/selftests/kselftest.h     | 14 ++++++
 2 files changed, 66 insertions(+), 35 deletions(-)
---
base-commit: 6465e260f48790807eef06b583b38ca9789b6072
change-id: 20230928-ktap-exec-45ea8d28309a

Best regards,
-- 
Mark Brown <broonie@kernel.org>

[PATCH 1/2] kselftest: Add a ksft_perror() helper

[Mark Brown]

The standard library perror() function provides a convenient way to print
an error message based on the current errno but this doesn't play nicely
with KTAP output. Provide a helper which does an equivalent thing in a KTAP
compatible format.

nolibc doesn't have a strerror() and adding the table of strings required
doesn't seem like a good fit for what it's trying to do so when we're using
that only print the errno.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/kselftest.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index 529d29a35900..af9f1202d423 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -48,6 +48,7 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <stdarg.h>
+#include <string.h>
 #include <stdio.h>
 #endif
 
@@ -155,6 +156,19 @@ static inline void ksft_print_msg(const char *msg, ...)
 	va_end(args);
 }
 
+static inline void ksft_perror(const char *msg)
+{
+#ifndef NOLIBC
+	ksft_print_msg("%s: %s (%d)\n", msg, strerror(errno), errno);
+#else
+	/*
+	 * nolibc doesn't provide strerror() and it seems
+	 * inappropriate to add one, just print the errno.
+	 */
+	ksft_print_msg("%s: %d)\n", msg, errno);
+#endif
+}
+
 static inline void ksft_test_result_pass(const char *msg, ...)
 {
 	int saved_errno = errno;

-- 
2.39.2

Re: [PATCH 1/2] kselftest: Add a ksft_perror() helper

[Kees Cook]

On Thu, Sep 28, 2023 at 04:38:11PM +0200, Mark Brown wrote:
> The standard library perror() function provides a convenient way to print
> an error message based on the current errno but this doesn't play nicely
> with KTAP output. Provide a helper which does an equivalent thing in a KTAP
> compatible format.
> 
> nolibc doesn't have a strerror() and adding the table of strings required
> doesn't seem like a good fit for what it's trying to do so when we're using
> that only print the errno.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Oh, interesting... what environment ends up without strerror()?

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

-- 
Kees Cook

Re: [PATCH 1/2] kselftest: Add a ksft_perror() helper

[Mark Brown]

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

On Thu, Sep 28, 2023 at 05:48:22PM -0700, Kees Cook wrote:

> > nolibc doesn't have a strerror() and adding the table of strings required
> > doesn't seem like a good fit for what it's trying to do so when we're using
> > that only print the errno.

> Oh, interesting... what environment ends up without strerror()?

Like I say it's for nolibc - it's just some header files (all in the
kernel source), while it generally aims to be libc compatible it's
intentionally very small.

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

Re: [PATCH 1/2] kselftest: Add a ksft_perror() helper

[Kees Cook]

On Fri, Sep 29, 2023 at 09:50:53AM +0200, Mark Brown wrote:
> On Thu, Sep 28, 2023 at 05:48:22PM -0700, Kees Cook wrote:
> 
> > > nolibc doesn't have a strerror() and adding the table of strings required
> > > doesn't seem like a good fit for what it's trying to do so when we're using
> > > that only print the errno.
> 
> > Oh, interesting... what environment ends up without strerror()?
> 
> Like I say it's for nolibc - it's just some header files (all in the
> kernel source), while it generally aims to be libc compatible it's
> intentionally very small.

Right, I mean, how would one normally encounter this environment? Running
the selftests on m68k userspace or something?

-- 
Kees Cook

Re: [PATCH 1/2] kselftest: Add a ksft_perror() helper

[Mark Brown]

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

On Fri, Sep 29, 2023 at 10:31:56AM -0700, Kees Cook wrote:
> On Fri, Sep 29, 2023 at 09:50:53AM +0200, Mark Brown wrote:

> > Like I say it's for nolibc - it's just some header files (all in the
> > kernel source), while it generally aims to be libc compatible it's
> > intentionally very small.

> Right, I mean, how would one normally encounter this environment? Running
> the selftests on m68k userspace or something?

There's a bunch of selftests that cover interfaces that are intended to
be used by libc which are built with nolibc in order to avoid the tests
and glibc stomping over each other.

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

[PATCH 2/2] selftests/exec: Convert execveat test to generate KTAP output

[Mark Brown]

Currently the execveat test does not produce KTAP output but rather a
custom format. This means that we only get a pass/fail for the suite, not
for each individual test that the suite does. Convert to using the standard
kselftest output functions which result in KTAP output being generated.

The main trick with this is that, being an exec() related test, the
program executes itself and returns specific exit codes to verify
success meaning that we need to only use the top level kselftest
header/summary functions when invoked directly rather than when run as
part of a test.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/exec/execveat.c | 87 ++++++++++++++++++++-------------
 1 file changed, 52 insertions(+), 35 deletions(-)

diff --git a/tools/testing/selftests/exec/execveat.c b/tools/testing/selftests/exec/execveat.c
index 67bf7254a48f..bf79d664c8e6 100644
--- a/tools/testing/selftests/exec/execveat.c
+++ b/tools/testing/selftests/exec/execveat.c
@@ -23,6 +23,9 @@
 
 #include "../kselftest.h"
 
+#define TESTS_EXPECTED 51
+#define TEST_NAME_LEN (PATH_MAX * 4)
+
 static char longpath[2 * PATH_MAX] = "";
 static char *envp[] = { "IN_TEST=yes", NULL, NULL };
 static char *argv[] = { "execveat", "99", NULL };
@@ -43,71 +46,85 @@ static int execveat_(int fd, const char *path, char **argv, char **envp,
 static int _check_execveat_fail(int fd, const char *path, int flags,
 				int expected_errno, const char *errno_str)
 {
+	char test_name[TEST_NAME_LEN];
 	int rc;
 
 	errno = 0;
-	printf("Check failure of execveat(%d, '%s', %d) with %s... ",
-		fd, path?:"(null)", flags, errno_str);
+	snprintf(test_name, sizeof(test_name),
+		 "Check failure of execveat(%d, '%s', %d) with %s",
+		 fd, path?:"(null)", flags, errno_str);
 	rc = execveat_(fd, path, argv, envp, flags);
 
 	if (rc > 0) {
-		printf("[FAIL] (unexpected success from execveat(2))\n");
+		ksft_print_msg("unexpected success from execveat(2)\n");
+		ksft_test_result_fail("%s\n", test_name);
 		return 1;
 	}
 	if (errno != expected_errno) {
-		printf("[FAIL] (expected errno %d (%s) not %d (%s)\n",
-			expected_errno, strerror(expected_errno),
-			errno, strerror(errno));
+		ksft_print_msg("expected errno %d (%s) not %d (%s)\n",
+			       expected_errno, strerror(expected_errno),
+			       errno, strerror(errno));
+		ksft_test_result_fail("%s\n", test_name);
 		return 1;
 	}
-	printf("[OK]\n");
+	ksft_test_result_pass("%s\n", test_name);
 	return 0;
 }
 
 static int check_execveat_invoked_rc(int fd, const char *path, int flags,
 				     int expected_rc, int expected_rc2)
 {
+	char test_name[TEST_NAME_LEN];
 	int status;
 	int rc;
 	pid_t child;
 	int pathlen = path ? strlen(path) : 0;
 
 	if (pathlen > 40)
-		printf("Check success of execveat(%d, '%.20s...%s', %d)... ",
-			fd, path, (path + pathlen - 20), flags);
+		snprintf(test_name, sizeof(test_name),
+			 "Check success of execveat(%d, '%.20s...%s', %d)... ",
+			 fd, path, (path + pathlen - 20), flags);
 	else
-		printf("Check success of execveat(%d, '%s', %d)... ",
-			fd, path?:"(null)", flags);
+		snprintf(test_name, sizeof(test_name),
+			 "Check success of execveat(%d, '%s', %d)... ",
+			 fd, path?:"(null)", flags);
+
 	child = fork();
 	if (child < 0) {
-		printf("[FAIL] (fork() failed)\n");
+		ksft_perror("fork() failed");
+		ksft_test_result_fail("%s\n", test_name);
 		return 1;
 	}
 	if (child == 0) {
 		/* Child: do execveat(). */
 		rc = execveat_(fd, path, argv, envp, flags);
-		printf("[FAIL]: execveat() failed, rc=%d errno=%d (%s)\n",
-			rc, errno, strerror(errno));
+		ksft_print_msg("execveat() failed, rc=%d errno=%d (%s)\n",
+			       rc, errno, strerror(errno));
+		ksft_test_result_fail("%s\n", test_name);
 		exit(1);  /* should not reach here */
 	}
 	/* Parent: wait for & check child's exit status. */
 	rc = waitpid(child, &status, 0);
 	if (rc != child) {
-		printf("[FAIL] (waitpid(%d,...) returned %d)\n", child, rc);
+		ksft_print_msg("waitpid(%d,...) returned %d\n", child, rc);
+		ksft_test_result_fail("%s\n", test_name);
 		return 1;
 	}
 	if (!WIFEXITED(status)) {
-		printf("[FAIL] (child %d did not exit cleanly, status=%08x)\n",
-			child, status);
+		ksft_print_msg("child %d did not exit cleanly, status=%08x\n",
+			       child, status);
+		ksft_test_result_fail("%s\n", test_name);
 		return 1;
 	}
 	if ((WEXITSTATUS(status) != expected_rc) &&
 	    (WEXITSTATUS(status) != expected_rc2)) {
-		printf("[FAIL] (child %d exited with %d not %d nor %d)\n",
-			child, WEXITSTATUS(status), expected_rc, expected_rc2);
+		ksft_print_msg("child %d exited with %d not %d nor %d\n",
+			       child, WEXITSTATUS(status), expected_rc,
+			       expected_rc2);
+		ksft_test_result_fail("%s\n", test_name);
 		return 1;
 	}
-	printf("[OK]\n");
+	ksft_test_result_pass("%s\n", test_name);
 	return 0;
 }
 
@@ -129,11 +146,9 @@ static int open_or_die(const char *filename, int flags)
 {
 	int fd = open(filename, flags);
 
-	if (fd < 0) {
-		printf("Failed to open '%s'; "
+	if (fd < 0)
+		ksft_exit_fail_msg("Failed to open '%s'; "
 			"check prerequisites are available\n", filename);
-		exit(1);
-	}
 	return fd;
 }
 
@@ -162,8 +177,7 @@ static int check_execveat_pathmax(int root_dfd, const char *src, int is_script)
 		char *cwd = getcwd(NULL, 0);
 
 		if (!cwd) {
-			printf("Failed to getcwd(), errno=%d (%s)\n",
-			       errno, strerror(errno));
+			ksft_perror("Failed to getcwd()");
 			return 2;
 		}
 		strcpy(longpath, cwd);
@@ -193,12 +207,12 @@ static int check_execveat_pathmax(int root_dfd, const char *src, int is_script)
 	 */
 	fd = open(longpath, O_RDONLY);
 	if (fd > 0) {
-		printf("Invoke copy of '%s' via filename of length %zu:\n",
-			src, strlen(longpath));
+		ksft_print_msg("Invoke copy of '%s' via filename of length %zu:\n",
+			       src, strlen(longpath));
 		fail += check_execveat(fd, "", AT_EMPTY_PATH);
 	} else {
-		printf("Failed to open length %zu filename, errno=%d (%s)\n",
-			strlen(longpath), errno, strerror(errno));
+		ksft_print_msg("Failed to open length %zu filename, errno=%d (%s)\n",
+			       strlen(longpath), errno, strerror(errno));
 		fail++;
 	}
 
@@ -405,28 +419,31 @@ int main(int argc, char **argv)
 		const char *in_test = getenv("IN_TEST");
 
 		if (verbose) {
-			printf("  invoked with:");
+			ksft_print_msg("invoked with:\n");
 			for (ii = 0; ii < argc; ii++)
-				printf(" [%d]='%s'", ii, argv[ii]);
-			printf("\n");
+				ksft_print_msg("\t[%d]='%s\n'", ii, argv[ii]);
 		}
 
 		/* Check expected environment transferred. */
 		if (!in_test || strcmp(in_test, "yes") != 0) {
-			printf("[FAIL] (no IN_TEST=yes in env)\n");
+			ksft_print_msg("no IN_TEST=yes in env\n");
 			return 1;
 		}
 
 		/* Use the final argument as an exit code. */
 		rc = atoi(argv[argc - 1]);
-		fflush(stdout);
+		exit(rc);
 	} else {
+		ksft_print_header();
+		ksft_set_plan(TESTS_EXPECTED);
 		prerequisites();
 		if (verbose)
 			envp[1] = "VERBOSE=1";
 		rc = run_tests();
 		if (rc > 0)
 			printf("%d tests failed\n", rc);
+		ksft_finished();
 	}
+
 	return rc;
 }

-- 
2.39.2

Re: [PATCH 2/2] selftests/exec: Convert execveat test to generate KTAP output

[Kees Cook]

On Thu, Sep 28, 2023 at 04:38:12PM +0200, Mark Brown wrote:
> Currently the execveat test does not produce KTAP output but rather a
> custom format. This means that we only get a pass/fail for the suite, not
> for each individual test that the suite does. Convert to using the standard
> kselftest output functions which result in KTAP output being generated.
> 
> The main trick with this is that, being an exec() related test, the
> program executes itself and returns specific exit codes to verify
> success meaning that we need to only use the top level kselftest
> header/summary functions when invoked directly rather than when run as
> part of a test.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Yay! More KTAP! :)

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

-- 
Kees Cook