[PATCH 0/7] selftests/harness: Switch to TAP output

[PATCH 0/7] selftests/harness: Switch to TAP output

[Kees Cook]

Hi,

I finally got around to converting the kselftest_harness.h API to actually
use the kselftest.h API so all the tools using it can actually report
TAP correctly. As part of this, there are a bunch of related cleanups,
API updates, and additions.

Thanks!

-Kees

Kees Cook (7):
  selftests/clone3: Reorder reporting output
  selftests: Remove unneeded selftest API headers
  selftests/binderfs: Fix harness API usage
  selftests: Add header documentation and helpers
  selftests/harness: Switch to TAP output
  selftests/harness: Fully track XFAIL reports
  selftests/harness: Display signed values correctly

 tools/testing/selftests/clone3/clone3.c       |   2 +-
 .../selftests/clone3/clone3_clear_sighand.c   |   3 +-
 .../testing/selftests/clone3/clone3_set_tid.c |   2 +-
 .../filesystems/binderfs/binderfs_test.c      | 284 +++++++++---------
 tools/testing/selftests/kselftest.h           |  78 ++++-
 tools/testing/selftests/kselftest_harness.h   | 142 ++++++---
 .../pid_namespace/regression_enomem.c         |   1 -
 .../selftests/pidfd/pidfd_getfd_test.c        |   1 -
 .../selftests/pidfd/pidfd_setns_test.c        |   1 -
 .../selftests/uevent/uevent_filtering.c       |   1 -
 10 files changed, 330 insertions(+), 185 deletions(-)

-- 
2.25.1

[PATCH 1/7] selftests/clone3: Reorder reporting output

[Kees Cook]

Selftest output reporting was happening before the TAP headers and plan
had been emitted. Move the first test reports later.

Cc: Christian Brauner <christian@brauner.io>
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/clone3/clone3.c               | 2 +-
 tools/testing/selftests/clone3/clone3_clear_sighand.c | 3 +--
 tools/testing/selftests/clone3/clone3_set_tid.c       | 2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c
index f14c269a5a18..b7e6dec36173 100644
--- a/tools/testing/selftests/clone3/clone3.c
+++ b/tools/testing/selftests/clone3/clone3.c
@@ -131,9 +131,9 @@ int main(int argc, char *argv[])
 
 	uid_t uid = getuid();
 
-	test_clone3_supported();
 	ksft_print_header();
 	ksft_set_plan(17);
+	test_clone3_supported();
 
 	/* Just a simple clone3() should return 0.*/
 	test_clone3(0, 0, 0, CLONE3_ARGS_NO_TEST);
diff --git a/tools/testing/selftests/clone3/clone3_clear_sighand.c b/tools/testing/selftests/clone3/clone3_clear_sighand.c
index 9e1af8aa7698..db5fc9c5edcf 100644
--- a/tools/testing/selftests/clone3/clone3_clear_sighand.c
+++ b/tools/testing/selftests/clone3/clone3_clear_sighand.c
@@ -119,9 +119,8 @@ static void test_clone3_clear_sighand(void)
 int main(int argc, char **argv)
 {
 	ksft_print_header();
-	test_clone3_supported();
-
 	ksft_set_plan(1);
+	test_clone3_supported();
 
 	test_clone3_clear_sighand();
 
diff --git a/tools/testing/selftests/clone3/clone3_set_tid.c b/tools/testing/selftests/clone3/clone3_set_tid.c
index 25beb22f35b5..5831c1082d6d 100644
--- a/tools/testing/selftests/clone3/clone3_set_tid.c
+++ b/tools/testing/selftests/clone3/clone3_set_tid.c
@@ -157,8 +157,8 @@ int main(int argc, char *argv[])
 	pid_t set_tid[MAX_PID_NS_LEVEL * 2];
 
 	ksft_print_header();
-	test_clone3_supported();
 	ksft_set_plan(29);
+	test_clone3_supported();
 
 	if (pipe(pipe_1) < 0 || pipe(pipe_2) < 0)
 		ksft_exit_fail_msg("pipe() failed\n");
-- 
2.25.1

Re: [PATCH 1/7] selftests/clone3: Reorder reporting output

[Christian Brauner]

On Thu, Jun 11, 2020 at 03:40:22PM -0700, Kees Cook wrote:
> Selftest output reporting was happening before the TAP headers and plan
> had been emitted. Move the first test reports later.
> 
> Cc: Christian Brauner <christian@brauner.io>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: linux-kselftest@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

Again, much appreciated:

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

>  tools/testing/selftests/clone3/clone3.c               | 2 +-
>  tools/testing/selftests/clone3/clone3_clear_sighand.c | 3 +--
>  tools/testing/selftests/clone3/clone3_set_tid.c       | 2 +-
>  3 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c
> index f14c269a5a18..b7e6dec36173 100644
> --- a/tools/testing/selftests/clone3/clone3.c
> +++ b/tools/testing/selftests/clone3/clone3.c
> @@ -131,9 +131,9 @@ int main(int argc, char *argv[])
>  
>  	uid_t uid = getuid();
>  
> -	test_clone3_supported();
>  	ksft_print_header();
>  	ksft_set_plan(17);
> +	test_clone3_supported();
>  
>  	/* Just a simple clone3() should return 0.*/
>  	test_clone3(0, 0, 0, CLONE3_ARGS_NO_TEST);
> diff --git a/tools/testing/selftests/clone3/clone3_clear_sighand.c b/tools/testing/selftests/clone3/clone3_clear_sighand.c
> index 9e1af8aa7698..db5fc9c5edcf 100644
> --- a/tools/testing/selftests/clone3/clone3_clear_sighand.c
> +++ b/tools/testing/selftests/clone3/clone3_clear_sighand.c
> @@ -119,9 +119,8 @@ static void test_clone3_clear_sighand(void)
>  int main(int argc, char **argv)
>  {
>  	ksft_print_header();
> -	test_clone3_supported();
> -
>  	ksft_set_plan(1);
> +	test_clone3_supported();
>  
>  	test_clone3_clear_sighand();
>  
> diff --git a/tools/testing/selftests/clone3/clone3_set_tid.c b/tools/testing/selftests/clone3/clone3_set_tid.c
> index 25beb22f35b5..5831c1082d6d 100644
> --- a/tools/testing/selftests/clone3/clone3_set_tid.c
> +++ b/tools/testing/selftests/clone3/clone3_set_tid.c
> @@ -157,8 +157,8 @@ int main(int argc, char *argv[])
>  	pid_t set_tid[MAX_PID_NS_LEVEL * 2];
>  
>  	ksft_print_header();
> -	test_clone3_supported();
>  	ksft_set_plan(29);
> +	test_clone3_supported();
>  
>  	if (pipe(pipe_1) < 0 || pipe(pipe_2) < 0)
>  		ksft_exit_fail_msg("pipe() failed\n");
> -- 
> 2.25.1
> 

[PATCH 2/7] selftests: Remove unneeded selftest API headers

[Kees Cook]

Remove unused includes of the kselftest.h header.

Cc: Christian Brauner <christian@brauner.io>
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/pid_namespace/regression_enomem.c | 1 -
 tools/testing/selftests/pidfd/pidfd_getfd_test.c          | 1 -
 tools/testing/selftests/pidfd/pidfd_setns_test.c          | 1 -
 tools/testing/selftests/uevent/uevent_filtering.c         | 1 -
 4 files changed, 4 deletions(-)

diff --git a/tools/testing/selftests/pid_namespace/regression_enomem.c b/tools/testing/selftests/pid_namespace/regression_enomem.c
index 73d532556d17..7d84097ad45c 100644
--- a/tools/testing/selftests/pid_namespace/regression_enomem.c
+++ b/tools/testing/selftests/pid_namespace/regression_enomem.c
@@ -11,7 +11,6 @@
 #include <syscall.h>
 #include <sys/wait.h>
 
-#include "../kselftest.h"
 #include "../kselftest_harness.h"
 #include "../pidfd/pidfd.h"
 
diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
index 401a7c1d0312..eecbf18510fd 100644
--- a/tools/testing/selftests/pidfd/pidfd_getfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
@@ -18,7 +18,6 @@
 #include <linux/kcmp.h>
 
 #include "pidfd.h"
-#include "../kselftest.h"
 #include "../kselftest_harness.h"
 
 /*
diff --git a/tools/testing/selftests/pidfd/pidfd_setns_test.c b/tools/testing/selftests/pidfd/pidfd_setns_test.c
index 133ec5b6cda8..f66861cf9c4d 100644
--- a/tools/testing/selftests/pidfd/pidfd_setns_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_setns_test.c
@@ -20,7 +20,6 @@
 
 #include "pidfd.h"
 #include "../clone3/clone3_selftests.h"
-#include "../kselftest.h"
 #include "../kselftest_harness.h"
 
 enum {
diff --git a/tools/testing/selftests/uevent/uevent_filtering.c b/tools/testing/selftests/uevent/uevent_filtering.c
index f83391aa42cf..5cebfb356345 100644
--- a/tools/testing/selftests/uevent/uevent_filtering.c
+++ b/tools/testing/selftests/uevent/uevent_filtering.c
@@ -19,7 +19,6 @@
 #include <sys/wait.h>
 #include <unistd.h>
 
-#include "../kselftest.h"
 #include "../kselftest_harness.h"
 
 #define __DEV_FULL "/sys/devices/virtual/mem/full/uevent"
-- 
2.25.1

Re: [PATCH 2/7] selftests: Remove unneeded selftest API headers

[Christian Brauner]

On Thu, Jun 11, 2020 at 03:40:23PM -0700, Kees Cook wrote:
> Remove unused includes of the kselftest.h header.
> 
> Cc: Christian Brauner <christian@brauner.io>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: linux-kselftest@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

Thanks!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

>  tools/testing/selftests/pid_namespace/regression_enomem.c | 1 -
>  tools/testing/selftests/pidfd/pidfd_getfd_test.c          | 1 -
>  tools/testing/selftests/pidfd/pidfd_setns_test.c          | 1 -
>  tools/testing/selftests/uevent/uevent_filtering.c         | 1 -
>  4 files changed, 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/pid_namespace/regression_enomem.c b/tools/testing/selftests/pid_namespace/regression_enomem.c
> index 73d532556d17..7d84097ad45c 100644
> --- a/tools/testing/selftests/pid_namespace/regression_enomem.c
> +++ b/tools/testing/selftests/pid_namespace/regression_enomem.c
> @@ -11,7 +11,6 @@
>  #include <syscall.h>
>  #include <sys/wait.h>
>  
> -#include "../kselftest.h"
>  #include "../kselftest_harness.h"
>  #include "../pidfd/pidfd.h"
>  
> diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
> index 401a7c1d0312..eecbf18510fd 100644
> --- a/tools/testing/selftests/pidfd/pidfd_getfd_test.c
> +++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
> @@ -18,7 +18,6 @@
>  #include <linux/kcmp.h>
>  
>  #include "pidfd.h"
> -#include "../kselftest.h"
>  #include "../kselftest_harness.h"
>  
>  /*
> diff --git a/tools/testing/selftests/pidfd/pidfd_setns_test.c b/tools/testing/selftests/pidfd/pidfd_setns_test.c
> index 133ec5b6cda8..f66861cf9c4d 100644
> --- a/tools/testing/selftests/pidfd/pidfd_setns_test.c
> +++ b/tools/testing/selftests/pidfd/pidfd_setns_test.c
> @@ -20,7 +20,6 @@
>  
>  #include "pidfd.h"
>  #include "../clone3/clone3_selftests.h"
> -#include "../kselftest.h"
>  #include "../kselftest_harness.h"
>  
>  enum {
> diff --git a/tools/testing/selftests/uevent/uevent_filtering.c b/tools/testing/selftests/uevent/uevent_filtering.c
> index f83391aa42cf..5cebfb356345 100644
> --- a/tools/testing/selftests/uevent/uevent_filtering.c
> +++ b/tools/testing/selftests/uevent/uevent_filtering.c
> @@ -19,7 +19,6 @@
>  #include <sys/wait.h>
>  #include <unistd.h>
>  
> -#include "../kselftest.h"
>  #include "../kselftest_harness.h"
>  
>  #define __DEV_FULL "/sys/devices/virtual/mem/full/uevent"
> -- 
> 2.25.1
> 

[PATCH 3/7] selftests/binderfs: Fix harness API usage

[Kees Cook]

The binderfs test mixed the full harness API and the selftest API.
Adjust to use only the harness API so that the harness API can switch
to using the selftest API internally in future patches.

Cc: Shuah Khan <shuah@kernel.org>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 .../filesystems/binderfs/binderfs_test.c      | 284 +++++++++---------
 1 file changed, 146 insertions(+), 138 deletions(-)

diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
index 8a6b507e34a8..1d27f52c61e6 100644
--- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -21,7 +21,6 @@
 #include <linux/android/binder.h>
 #include <linux/android/binderfs.h>
 
-#include "../../kselftest.h"
 #include "../../kselftest_harness.h"
 
 #define DEFAULT_THREADS 4
@@ -37,37 +36,26 @@
 		fd = -EBADF;        \
 	}
 
-#define log_exit(format, ...)                                                  \
-	({                                                                     \
-		fprintf(stderr, format "\n", ##__VA_ARGS__);                   \
-		exit(EXIT_FAILURE);                                            \
-	})
-
-static void change_mountns(void)
+static void change_mountns(struct __test_metadata *_metadata)
 {
 	int ret;
 
 	ret = unshare(CLONE_NEWNS);
-	if (ret < 0)
-		ksft_exit_fail_msg("%s - Failed to unshare mount namespace\n",
-				   strerror(errno));
+	ASSERT_EQ(ret, 0) {
+		TH_LOG("%s - Failed to unshare mount namespace",
+			strerror(errno));
+	}
 
 	ret = mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, 0);
-	if (ret < 0)
-		ksft_exit_fail_msg("%s - Failed to mount / as private\n",
-				   strerror(errno));
-}
-
-static void rmdir_protect_errno(const char *dir)
-{
-	int saved_errno = errno;
-	(void)rmdir(dir);
-	errno = saved_errno;
+	ASSERT_EQ(ret, 0) {
+		TH_LOG("%s - Failed to mount / as private",
+			strerror(errno));
+	}
 }
 
-static int __do_binderfs_test(void)
+static int __do_binderfs_test(struct __test_metadata *_metadata)
 {
-	int fd, ret, saved_errno;
+	int fd, ret, saved_errno, result = 1;
 	size_t len;
 	ssize_t wret;
 	struct binderfs_device device = { 0 };
@@ -75,113 +63,107 @@ static int __do_binderfs_test(void)
 	char binderfs_mntpt[] = P_tmpdir "/binderfs_XXXXXX",
 		device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") + BINDERFS_MAX_NAME];
 
-	change_mountns();
+	change_mountns(_metadata);
 
-	if (!mkdtemp(binderfs_mntpt))
-		ksft_exit_fail_msg(
-			"%s - Failed to create binderfs mountpoint\n",
+	EXPECT_NE(mkdtemp(binderfs_mntpt), NULL) {
+		TH_LOG("%s - Failed to create binderfs mountpoint",
 			strerror(errno));
+		goto out;
+	}
 
 	ret = mount(NULL, binderfs_mntpt, "binder", 0, 0);
-	if (ret < 0) {
-		if (errno != ENODEV)
-			ksft_exit_fail_msg("%s - Failed to mount binderfs\n",
-					   strerror(errno));
-
-		rmdir_protect_errno(binderfs_mntpt);
-		return 1;
+	EXPECT_EQ(ret, 0) {
+		if (errno == ENODEV)
+			XFAIL(goto out, "binderfs missing");
+		TH_LOG("%s - Failed to mount binderfs", strerror(errno));
+		goto rmdir;
 	}
 
-	/* binderfs mount test passed */
-	ksft_inc_pass_cnt();
+	/* success: binderfs mounted */
 
 	memcpy(device.name, "my-binder", strlen("my-binder"));
 
 	snprintf(device_path, sizeof(device_path), "%s/binder-control", binderfs_mntpt);
 	fd = open(device_path, O_RDONLY | O_CLOEXEC);
-	if (fd < 0)
-		ksft_exit_fail_msg(
-			"%s - Failed to open binder-control device\n",
+	EXPECT_GE(fd, 0) {
+		TH_LOG("%s - Failed to open binder-control device",
 			strerror(errno));
+		goto umount;
+	}
 
 	ret = ioctl(fd, BINDER_CTL_ADD, &device);
 	saved_errno = errno;
 	close(fd);
 	errno = saved_errno;
-	if (ret < 0) {
-		rmdir_protect_errno(binderfs_mntpt);
-		ksft_exit_fail_msg(
-			"%s - Failed to allocate new binder device\n",
+	EXPECT_GE(ret, 0) {
+		TH_LOG("%s - Failed to allocate new binder device",
 			strerror(errno));
+		goto umount;
 	}
 
-	ksft_print_msg(
-		"Allocated new binder device with major %d, minor %d, and name %s\n",
+	TH_LOG("Allocated new binder device with major %d, minor %d, and name %s",
 		device.major, device.minor, device.name);
 
-	/* binder device allocation test passed */
-	ksft_inc_pass_cnt();
+	/* success: binder device allocation */
 
 	snprintf(device_path, sizeof(device_path), "%s/my-binder", binderfs_mntpt);
 	fd = open(device_path, O_CLOEXEC | O_RDONLY);
-	if (fd < 0) {
-		rmdir_protect_errno(binderfs_mntpt);
-		ksft_exit_fail_msg("%s - Failed to open my-binder device\n",
-				   strerror(errno));
+	EXPECT_GE(fd, 0) {
+		TH_LOG("%s - Failed to open my-binder device",
+			strerror(errno));
+		goto umount;
 	}
 
 	ret = ioctl(fd, BINDER_VERSION, &version);
 	saved_errno = errno;
 	close(fd);
 	errno = saved_errno;
-	if (ret < 0) {
-		rmdir_protect_errno(binderfs_mntpt);
-		ksft_exit_fail_msg(
-			"%s - Failed to open perform BINDER_VERSION request\n",
+	EXPECT_GE(ret, 0) {
+		TH_LOG("%s - Failed to open perform BINDER_VERSION request",
 			strerror(errno));
+		goto umount;
 	}
 
-	ksft_print_msg("Detected binder version: %d\n",
-		       version.protocol_version);
+	TH_LOG("Detected binder version: %d", version.protocol_version);
 
-	/* binder transaction with binderfs binder device passed */
-	ksft_inc_pass_cnt();
+	/* success: binder transaction with binderfs binder device */
 
 	ret = unlink(device_path);
-	if (ret < 0) {
-		rmdir_protect_errno(binderfs_mntpt);
-		ksft_exit_fail_msg("%s - Failed to delete binder device\n",
-				   strerror(errno));
+	EXPECT_EQ(ret, 0) {
+		TH_LOG("%s - Failed to delete binder device",
+			strerror(errno));
+		goto umount;
 	}
 
-	/* binder device removal passed */
-	ksft_inc_pass_cnt();
+	/* success: binder device removal */
 
 	snprintf(device_path, sizeof(device_path), "%s/binder-control", binderfs_mntpt);
 	ret = unlink(device_path);
-	if (!ret) {
-		rmdir_protect_errno(binderfs_mntpt);
-		ksft_exit_fail_msg("Managed to delete binder-control device\n");
-	} else if (errno != EPERM) {
-		rmdir_protect_errno(binderfs_mntpt);
-		ksft_exit_fail_msg(
-			"%s - Failed to delete binder-control device but exited with unexpected error code\n",
+	EXPECT_NE(ret, 0) {
+		TH_LOG("Managed to delete binder-control device");
+		goto umount;
+	}
+	EXPECT_EQ(errno, EPERM) {
+		TH_LOG("%s - Failed to delete binder-control device but exited with unexpected error code",
 			strerror(errno));
+		goto umount;
 	}
 
-	/* binder-control device removal failed as expected */
-	ksft_inc_xfail_cnt();
+	/* success: binder-control device removal failed as expected */
+	result = 0;
 
-on_error:
+umount:
 	ret = umount2(binderfs_mntpt, MNT_DETACH);
-	rmdir_protect_errno(binderfs_mntpt);
-	if (ret < 0)
-		ksft_exit_fail_msg("%s - Failed to unmount binderfs\n",
-				   strerror(errno));
-
-	/* binderfs unmount test passed */
-	ksft_inc_pass_cnt();
-	return 0;
+	EXPECT_EQ(ret, 0) {
+		TH_LOG("%s - Failed to unmount binderfs", strerror(errno));
+	}
+rmdir:
+	ret = rmdir(binderfs_mntpt);
+	EXPECT_EQ(ret, 0) {
+		TH_LOG("%s - Failed to rmdir binderfs mount", strerror(errno));
+	}
+out:
+	return result;
 }
 
 static int wait_for_pid(pid_t pid)
@@ -291,7 +273,7 @@ static int write_id_mapping(enum idmap_type type, pid_t pid, const char *buf,
 	return 0;
 }
 
-static void change_userns(int syncfds[2])
+static void change_userns(struct __test_metadata *_metadata, int syncfds[2])
 {
 	int ret;
 	char buf;
@@ -299,25 +281,29 @@ static void change_userns(int syncfds[2])
 	close_prot_errno_disarm(syncfds[1]);
 
 	ret = unshare(CLONE_NEWUSER);
-	if (ret < 0)
-		ksft_exit_fail_msg("%s - Failed to unshare user namespace\n",
-				   strerror(errno));
+	ASSERT_EQ(ret, 0) {
+		TH_LOG("%s - Failed to unshare user namespace",
+			strerror(errno));
+	}
 
 	ret = write_nointr(syncfds[0], "1", 1);
-	if (ret != 1)
-		ksft_exit_fail_msg("write_nointr() failed\n");
+	ASSERT_EQ(ret, 1) {
+		TH_LOG("write_nointr() failed");
+	}
 
 	ret = read_nointr(syncfds[0], &buf, 1);
-	if (ret != 1)
-		ksft_exit_fail_msg("read_nointr() failed\n");
+	ASSERT_EQ(ret, 1) {
+		TH_LOG("read_nointr() failed");
+	}
 
 	close_prot_errno_disarm(syncfds[0]);
 
-	if (setid_userns_root())
-		ksft_exit_fail_msg("setid_userns_root() failed");
+	ASSERT_EQ(setid_userns_root(), 0) {
+		TH_LOG("setid_userns_root() failed");
+	}
 }
 
-static void change_idmaps(int syncfds[2], pid_t pid)
+static void change_idmaps(struct __test_metadata *_metadata, int syncfds[2], pid_t pid)
 {
 	int ret;
 	char buf;
@@ -326,35 +312,42 @@ static void change_idmaps(int syncfds[2], pid_t pid)
 	close_prot_errno_disarm(syncfds[0]);
 
 	ret = read_nointr(syncfds[1], &buf, 1);
-	if (ret != 1)
-		ksft_exit_fail_msg("read_nointr() failed\n");
+	ASSERT_EQ(ret, 1) {
+		TH_LOG("read_nointr() failed");
+	}
 
 	snprintf(id_map, sizeof(id_map), "0 %d 1\n", getuid());
 	ret = write_id_mapping(UID_MAP, pid, id_map, strlen(id_map));
-	if (ret)
-		ksft_exit_fail_msg("write_id_mapping(UID_MAP) failed");
+	ASSERT_EQ(ret, 0) {
+		TH_LOG("write_id_mapping(UID_MAP) failed");
+	}
 
 	snprintf(id_map, sizeof(id_map), "0 %d 1\n", getgid());
 	ret = write_id_mapping(GID_MAP, pid, id_map, strlen(id_map));
-	if (ret)
-		ksft_exit_fail_msg("write_id_mapping(GID_MAP) failed");
+	ASSERT_EQ(ret, 0) {
+		TH_LOG("write_id_mapping(GID_MAP) failed");
+	}
 
 	ret = write_nointr(syncfds[1], "1", 1);
-	if (ret != 1)
-		ksft_exit_fail_msg("write_nointr() failed");
+	ASSERT_EQ(ret, 1) {
+		TH_LOG("write_nointr() failed");
+	}
 
 	close_prot_errno_disarm(syncfds[1]);
 }
 
+struct __test_metadata *_thread_metadata;
 static void *binder_version_thread(void *data)
 {
+	struct __test_metadata *_metadata = _thread_metadata;
 	int fd = PTR_TO_INT(data);
 	struct binder_version version = { 0 };
 	int ret;
 
 	ret = ioctl(fd, BINDER_VERSION, &version);
 	if (ret < 0)
-		ksft_print_msg("%s - Failed to open perform BINDER_VERSION request\n", strerror(errno));
+		TH_LOG("%s - Failed to open perform BINDER_VERSION request\n",
+			strerror(errno));
 
 	pthread_exit(data);
 }
@@ -377,68 +370,79 @@ TEST(binderfs_stress)
 		device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") + BINDERFS_MAX_NAME];
 
 	ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
-	if (ret < 0)
-		ksft_exit_fail_msg("%s - Failed to create socket pair", strerror(errno));
+	ASSERT_EQ(ret, 0) {
+		TH_LOG("%s - Failed to create socket pair", strerror(errno));
+	}
 
 	pid = fork();
-	if (pid < 0) {
+	ASSERT_GE(pid, 0) {
+		TH_LOG("%s - Failed to fork", strerror(errno));
 		close_prot_errno_disarm(syncfds[0]);
 		close_prot_errno_disarm(syncfds[1]);
-		ksft_exit_fail_msg("%s - Failed to fork", strerror(errno));
 	}
 
 	if (pid == 0) {
 		int i, j, k, nthreads;
 		pthread_attr_t attr;
 		pthread_t threads[DEFAULT_THREADS];
-		change_userns(syncfds);
-		change_mountns();
+		change_userns(_metadata, syncfds);
+		change_mountns(_metadata);
 
-		if (!mkdtemp(binderfs_mntpt))
-			log_exit("%s - Failed to create binderfs mountpoint\n",
-				 strerror(errno));
+		ASSERT_NE(mkdtemp(binderfs_mntpt), NULL) {
+			TH_LOG("%s - Failed to create binderfs mountpoint",
+				strerror(errno));
+		}
 
 		ret = mount(NULL, binderfs_mntpt, "binder", 0, 0);
-		if (ret < 0)
-			log_exit("%s - Failed to mount binderfs\n", strerror(errno));
+		ASSERT_EQ(ret, 0) {
+			TH_LOG("%s - Failed to mount binderfs", strerror(errno));
+		}
 
 		for (int i = 0; i < ARRAY_SIZE(fds); i++) {
 
 			snprintf(device_path, sizeof(device_path),
 				 "%s/binder-control", binderfs_mntpt);
 			fd = open(device_path, O_RDONLY | O_CLOEXEC);
-			if (fd < 0)
-				log_exit("%s - Failed to open binder-control device\n", strerror(errno));
+			ASSERT_GE(fd, 0) {
+				TH_LOG("%s - Failed to open binder-control device",
+					strerror(errno));
+			}
 
 			memset(&device, 0, sizeof(device));
 			snprintf(device.name, sizeof(device.name), "%d", i);
 			ret = ioctl(fd, BINDER_CTL_ADD, &device);
 			close_prot_errno_disarm(fd);
-			if (ret < 0)
-				log_exit("%s - Failed to allocate new binder device\n", strerror(errno));
+			ASSERT_EQ(ret, 0) {
+				TH_LOG("%s - Failed to allocate new binder device",
+					strerror(errno));
+			}
 
 			snprintf(device_path, sizeof(device_path), "%s/%d",
 				 binderfs_mntpt, i);
 			fds[i] = open(device_path, O_RDONLY | O_CLOEXEC);
-			if (fds[i] < 0)
-				log_exit("%s - Failed to open binder device\n", strerror(errno));
+			ASSERT_GE(fds[i], 0) {
+				TH_LOG("%s - Failed to open binder device", strerror(errno));
+			}
 		}
 
 		ret = umount2(binderfs_mntpt, MNT_DETACH);
-		rmdir_protect_errno(binderfs_mntpt);
-		if (ret < 0)
-			log_exit("%s - Failed to unmount binderfs\n", strerror(errno));
+		ASSERT_EQ(ret, 0) {
+			TH_LOG("%s - Failed to unmount binderfs", strerror(errno));
+			rmdir(binderfs_mntpt);
+		}
 
 		nthreads = get_nprocs_conf();
 		if (nthreads > DEFAULT_THREADS)
 			nthreads = DEFAULT_THREADS;
 
+		_thread_metadata = _metadata;
 		pthread_attr_init(&attr);
 		for (k = 0; k < ARRAY_SIZE(fds); k++) {
 			for (i = 0; i < nthreads; i++) {
 				ret = pthread_create(&threads[i], &attr, binder_version_thread, INT_TO_PTR(fds[k]));
 				if (ret) {
-					ksft_print_msg("%s - Failed to create thread %d\n", strerror(errno), i);
+					TH_LOG("%s - Failed to create thread %d",
+						strerror(errno), i);
 					break;
 				}
 			}
@@ -448,7 +452,8 @@ TEST(binderfs_stress)
 
 				ret = pthread_join(threads[j], &fdptr);
 				if (ret)
-					ksft_print_msg("%s - Failed to join thread %d for fd %d\n", strerror(errno), j, PTR_TO_INT(fdptr));
+					TH_LOG("%s - Failed to join thread %d for fd %d",
+						strerror(errno), j, PTR_TO_INT(fdptr));
 			}
 		}
 		pthread_attr_destroy(&attr);
@@ -459,11 +464,12 @@ TEST(binderfs_stress)
 		exit(EXIT_SUCCESS);
 	}
 
-	change_idmaps(syncfds, pid);
+	change_idmaps(_metadata, syncfds, pid);
 
 	ret = wait_for_pid(pid);
-	if (ret)
-		ksft_exit_fail_msg("wait_for_pid() failed");
+	ASSERT_EQ(ret, 0) {
+		TH_LOG("wait_for_pid() failed");
+	}
 }
 
 TEST(binderfs_test_privileged)
@@ -471,7 +477,7 @@ TEST(binderfs_test_privileged)
 	if (geteuid() != 0)
 		XFAIL(return, "Tests are not run as root. Skipping privileged tests");
 
-	if (__do_binderfs_test() == 1)
+	if (__do_binderfs_test(_metadata))
 		XFAIL(return, "The Android binderfs filesystem is not available");
 }
 
@@ -482,31 +488,33 @@ TEST(binderfs_test_unprivileged)
 	pid_t pid;
 
 	ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
-	if (ret < 0)
-		ksft_exit_fail_msg("%s - Failed to create socket pair", strerror(errno));
+	ASSERT_EQ(ret, 0) {
+		TH_LOG("%s - Failed to create socket pair", strerror(errno));
+	}
 
 	pid = fork();
-	if (pid < 0) {
+	ASSERT_GE(pid, 0) {
 		close_prot_errno_disarm(syncfds[0]);
 		close_prot_errno_disarm(syncfds[1]);
-		ksft_exit_fail_msg("%s - Failed to fork", strerror(errno));
+		TH_LOG("%s - Failed to fork", strerror(errno));
 	}
 
 	if (pid == 0) {
-		change_userns(syncfds);
-		if (__do_binderfs_test() == 1)
+		change_userns(_metadata, syncfds);
+		if (__do_binderfs_test(_metadata))
 			exit(2);
 		exit(EXIT_SUCCESS);
 	}
 
-	change_idmaps(syncfds, pid);
+	change_idmaps(_metadata, syncfds, pid);
 
 	ret = wait_for_pid(pid);
 	if (ret) {
 		if (ret == 2)
 			XFAIL(return, "The Android binderfs filesystem is not available");
-		else
-			ksft_exit_fail_msg("wait_for_pid() failed");
+		ASSERT_EQ(ret, 0) {
+			TH_LOG("wait_for_pid() failed");
+		}
 	}
 }
 
-- 
2.25.1

Re: [PATCH 3/7] selftests/binderfs: Fix harness API usage

[Christian Brauner]

On Thu, Jun 11, 2020 at 03:40:24PM -0700, Kees Cook wrote:
> The binderfs test mixed the full harness API and the selftest API.
> Adjust to use only the harness API so that the harness API can switch
> to using the selftest API internally in future patches.
> 
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Christian Brauner <christian.brauner@ubuntu.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-kselftest@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

I had this on my TODO but never actually got around to it, so thanks!

In all honesty, I've done a "Does this overall look sane?" review.
Simply because I lack the time to do this in more detail right now but
I'm happy this work is done and so overall:

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

>  .../filesystems/binderfs/binderfs_test.c      | 284 +++++++++---------
>  1 file changed, 146 insertions(+), 138 deletions(-)
> 
> diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> index 8a6b507e34a8..1d27f52c61e6 100644
> --- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> @@ -21,7 +21,6 @@
>  #include <linux/android/binder.h>
>  #include <linux/android/binderfs.h>
>  
> -#include "../../kselftest.h"
>  #include "../../kselftest_harness.h"
>  
>  #define DEFAULT_THREADS 4
> @@ -37,37 +36,26 @@
>  		fd = -EBADF;        \
>  	}
>  
> -#define log_exit(format, ...)                                                  \
> -	({                                                                     \
> -		fprintf(stderr, format "\n", ##__VA_ARGS__);                   \
> -		exit(EXIT_FAILURE);                                            \
> -	})
> -
> -static void change_mountns(void)
> +static void change_mountns(struct __test_metadata *_metadata)
>  {
>  	int ret;
>  
>  	ret = unshare(CLONE_NEWNS);
> -	if (ret < 0)
> -		ksft_exit_fail_msg("%s - Failed to unshare mount namespace\n",
> -				   strerror(errno));
> +	ASSERT_EQ(ret, 0) {
> +		TH_LOG("%s - Failed to unshare mount namespace",
> +			strerror(errno));
> +	}
>  
>  	ret = mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, 0);
> -	if (ret < 0)
> -		ksft_exit_fail_msg("%s - Failed to mount / as private\n",
> -				   strerror(errno));
> -}
> -
> -static void rmdir_protect_errno(const char *dir)
> -{
> -	int saved_errno = errno;
> -	(void)rmdir(dir);
> -	errno = saved_errno;
> +	ASSERT_EQ(ret, 0) {
> +		TH_LOG("%s - Failed to mount / as private",
> +			strerror(errno));
> +	}
>  }
>  
> -static int __do_binderfs_test(void)
> +static int __do_binderfs_test(struct __test_metadata *_metadata)
>  {
> -	int fd, ret, saved_errno;
> +	int fd, ret, saved_errno, result = 1;
>  	size_t len;
>  	ssize_t wret;
>  	struct binderfs_device device = { 0 };
> @@ -75,113 +63,107 @@ static int __do_binderfs_test(void)
>  	char binderfs_mntpt[] = P_tmpdir "/binderfs_XXXXXX",
>  		device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") + BINDERFS_MAX_NAME];
>  
> -	change_mountns();
> +	change_mountns(_metadata);
>  
> -	if (!mkdtemp(binderfs_mntpt))
> -		ksft_exit_fail_msg(
> -			"%s - Failed to create binderfs mountpoint\n",
> +	EXPECT_NE(mkdtemp(binderfs_mntpt), NULL) {
> +		TH_LOG("%s - Failed to create binderfs mountpoint",
>  			strerror(errno));
> +		goto out;
> +	}
>  
>  	ret = mount(NULL, binderfs_mntpt, "binder", 0, 0);
> -	if (ret < 0) {
> -		if (errno != ENODEV)
> -			ksft_exit_fail_msg("%s - Failed to mount binderfs\n",
> -					   strerror(errno));
> -
> -		rmdir_protect_errno(binderfs_mntpt);
> -		return 1;
> +	EXPECT_EQ(ret, 0) {
> +		if (errno == ENODEV)
> +			XFAIL(goto out, "binderfs missing");
> +		TH_LOG("%s - Failed to mount binderfs", strerror(errno));
> +		goto rmdir;
>  	}
>  
> -	/* binderfs mount test passed */
> -	ksft_inc_pass_cnt();
> +	/* success: binderfs mounted */
>  
>  	memcpy(device.name, "my-binder", strlen("my-binder"));
>  
>  	snprintf(device_path, sizeof(device_path), "%s/binder-control", binderfs_mntpt);
>  	fd = open(device_path, O_RDONLY | O_CLOEXEC);
> -	if (fd < 0)
> -		ksft_exit_fail_msg(
> -			"%s - Failed to open binder-control device\n",
> +	EXPECT_GE(fd, 0) {
> +		TH_LOG("%s - Failed to open binder-control device",
>  			strerror(errno));
> +		goto umount;
> +	}
>  
>  	ret = ioctl(fd, BINDER_CTL_ADD, &device);
>  	saved_errno = errno;
>  	close(fd);
>  	errno = saved_errno;
> -	if (ret < 0) {
> -		rmdir_protect_errno(binderfs_mntpt);
> -		ksft_exit_fail_msg(
> -			"%s - Failed to allocate new binder device\n",
> +	EXPECT_GE(ret, 0) {
> +		TH_LOG("%s - Failed to allocate new binder device",
>  			strerror(errno));
> +		goto umount;
>  	}
>  
> -	ksft_print_msg(
> -		"Allocated new binder device with major %d, minor %d, and name %s\n",
> +	TH_LOG("Allocated new binder device with major %d, minor %d, and name %s",
>  		device.major, device.minor, device.name);
>  
> -	/* binder device allocation test passed */
> -	ksft_inc_pass_cnt();
> +	/* success: binder device allocation */
>  
>  	snprintf(device_path, sizeof(device_path), "%s/my-binder", binderfs_mntpt);
>  	fd = open(device_path, O_CLOEXEC | O_RDONLY);
> -	if (fd < 0) {
> -		rmdir_protect_errno(binderfs_mntpt);
> -		ksft_exit_fail_msg("%s - Failed to open my-binder device\n",
> -				   strerror(errno));
> +	EXPECT_GE(fd, 0) {
> +		TH_LOG("%s - Failed to open my-binder device",
> +			strerror(errno));
> +		goto umount;
>  	}
>  
>  	ret = ioctl(fd, BINDER_VERSION, &version);
>  	saved_errno = errno;
>  	close(fd);
>  	errno = saved_errno;
> -	if (ret < 0) {
> -		rmdir_protect_errno(binderfs_mntpt);
> -		ksft_exit_fail_msg(
> -			"%s - Failed to open perform BINDER_VERSION request\n",
> +	EXPECT_GE(ret, 0) {
> +		TH_LOG("%s - Failed to open perform BINDER_VERSION request",
>  			strerror(errno));
> +		goto umount;
>  	}
>  
> -	ksft_print_msg("Detected binder version: %d\n",
> -		       version.protocol_version);
> +	TH_LOG("Detected binder version: %d", version.protocol_version);
>  
> -	/* binder transaction with binderfs binder device passed */
> -	ksft_inc_pass_cnt();
> +	/* success: binder transaction with binderfs binder device */
>  
>  	ret = unlink(device_path);
> -	if (ret < 0) {
> -		rmdir_protect_errno(binderfs_mntpt);
> -		ksft_exit_fail_msg("%s - Failed to delete binder device\n",
> -				   strerror(errno));
> +	EXPECT_EQ(ret, 0) {
> +		TH_LOG("%s - Failed to delete binder device",
> +			strerror(errno));
> +		goto umount;
>  	}
>  
> -	/* binder device removal passed */
> -	ksft_inc_pass_cnt();
> +	/* success: binder device removal */
>  
>  	snprintf(device_path, sizeof(device_path), "%s/binder-control", binderfs_mntpt);
>  	ret = unlink(device_path);
> -	if (!ret) {
> -		rmdir_protect_errno(binderfs_mntpt);
> -		ksft_exit_fail_msg("Managed to delete binder-control device\n");
> -	} else if (errno != EPERM) {
> -		rmdir_protect_errno(binderfs_mntpt);
> -		ksft_exit_fail_msg(
> -			"%s - Failed to delete binder-control device but exited with unexpected error code\n",
> +	EXPECT_NE(ret, 0) {
> +		TH_LOG("Managed to delete binder-control device");
> +		goto umount;
> +	}
> +	EXPECT_EQ(errno, EPERM) {
> +		TH_LOG("%s - Failed to delete binder-control device but exited with unexpected error code",
>  			strerror(errno));
> +		goto umount;
>  	}
>  
> -	/* binder-control device removal failed as expected */
> -	ksft_inc_xfail_cnt();
> +	/* success: binder-control device removal failed as expected */
> +	result = 0;
>  
> -on_error:
> +umount:
>  	ret = umount2(binderfs_mntpt, MNT_DETACH);
> -	rmdir_protect_errno(binderfs_mntpt);
> -	if (ret < 0)
> -		ksft_exit_fail_msg("%s - Failed to unmount binderfs\n",
> -				   strerror(errno));
> -
> -	/* binderfs unmount test passed */
> -	ksft_inc_pass_cnt();
> -	return 0;
> +	EXPECT_EQ(ret, 0) {
> +		TH_LOG("%s - Failed to unmount binderfs", strerror(errno));
> +	}
> +rmdir:
> +	ret = rmdir(binderfs_mntpt);
> +	EXPECT_EQ(ret, 0) {
> +		TH_LOG("%s - Failed to rmdir binderfs mount", strerror(errno));
> +	}
> +out:
> +	return result;
>  }
>  
>  static int wait_for_pid(pid_t pid)
> @@ -291,7 +273,7 @@ static int write_id_mapping(enum idmap_type type, pid_t pid, const char *buf,
>  	return 0;
>  }
>  
> -static void change_userns(int syncfds[2])
> +static void change_userns(struct __test_metadata *_metadata, int syncfds[2])
>  {
>  	int ret;
>  	char buf;
> @@ -299,25 +281,29 @@ static void change_userns(int syncfds[2])
>  	close_prot_errno_disarm(syncfds[1]);
>  
>  	ret = unshare(CLONE_NEWUSER);
> -	if (ret < 0)
> -		ksft_exit_fail_msg("%s - Failed to unshare user namespace\n",
> -				   strerror(errno));
> +	ASSERT_EQ(ret, 0) {
> +		TH_LOG("%s - Failed to unshare user namespace",
> +			strerror(errno));
> +	}
>  
>  	ret = write_nointr(syncfds[0], "1", 1);
> -	if (ret != 1)
> -		ksft_exit_fail_msg("write_nointr() failed\n");
> +	ASSERT_EQ(ret, 1) {
> +		TH_LOG("write_nointr() failed");
> +	}
>  
>  	ret = read_nointr(syncfds[0], &buf, 1);
> -	if (ret != 1)
> -		ksft_exit_fail_msg("read_nointr() failed\n");
> +	ASSERT_EQ(ret, 1) {
> +		TH_LOG("read_nointr() failed");
> +	}
>  
>  	close_prot_errno_disarm(syncfds[0]);
>  
> -	if (setid_userns_root())
> -		ksft_exit_fail_msg("setid_userns_root() failed");
> +	ASSERT_EQ(setid_userns_root(), 0) {
> +		TH_LOG("setid_userns_root() failed");
> +	}
>  }
>  
> -static void change_idmaps(int syncfds[2], pid_t pid)
> +static void change_idmaps(struct __test_metadata *_metadata, int syncfds[2], pid_t pid)
>  {
>  	int ret;
>  	char buf;
> @@ -326,35 +312,42 @@ static void change_idmaps(int syncfds[2], pid_t pid)
>  	close_prot_errno_disarm(syncfds[0]);
>  
>  	ret = read_nointr(syncfds[1], &buf, 1);
> -	if (ret != 1)
> -		ksft_exit_fail_msg("read_nointr() failed\n");
> +	ASSERT_EQ(ret, 1) {
> +		TH_LOG("read_nointr() failed");
> +	}
>  
>  	snprintf(id_map, sizeof(id_map), "0 %d 1\n", getuid());
>  	ret = write_id_mapping(UID_MAP, pid, id_map, strlen(id_map));
> -	if (ret)
> -		ksft_exit_fail_msg("write_id_mapping(UID_MAP) failed");
> +	ASSERT_EQ(ret, 0) {
> +		TH_LOG("write_id_mapping(UID_MAP) failed");
> +	}
>  
>  	snprintf(id_map, sizeof(id_map), "0 %d 1\n", getgid());
>  	ret = write_id_mapping(GID_MAP, pid, id_map, strlen(id_map));
> -	if (ret)
> -		ksft_exit_fail_msg("write_id_mapping(GID_MAP) failed");
> +	ASSERT_EQ(ret, 0) {
> +		TH_LOG("write_id_mapping(GID_MAP) failed");
> +	}
>  
>  	ret = write_nointr(syncfds[1], "1", 1);
> -	if (ret != 1)
> -		ksft_exit_fail_msg("write_nointr() failed");
> +	ASSERT_EQ(ret, 1) {
> +		TH_LOG("write_nointr() failed");
> +	}
>  
>  	close_prot_errno_disarm(syncfds[1]);
>  }
>  
> +struct __test_metadata *_thread_metadata;
>  static void *binder_version_thread(void *data)
>  {
> +	struct __test_metadata *_metadata = _thread_metadata;
>  	int fd = PTR_TO_INT(data);
>  	struct binder_version version = { 0 };
>  	int ret;
>  
>  	ret = ioctl(fd, BINDER_VERSION, &version);
>  	if (ret < 0)
> -		ksft_print_msg("%s - Failed to open perform BINDER_VERSION request\n", strerror(errno));
> +		TH_LOG("%s - Failed to open perform BINDER_VERSION request\n",
> +			strerror(errno));
>  
>  	pthread_exit(data);
>  }
> @@ -377,68 +370,79 @@ TEST(binderfs_stress)
>  		device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") + BINDERFS_MAX_NAME];
>  
>  	ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
> -	if (ret < 0)
> -		ksft_exit_fail_msg("%s - Failed to create socket pair", strerror(errno));
> +	ASSERT_EQ(ret, 0) {
> +		TH_LOG("%s - Failed to create socket pair", strerror(errno));
> +	}
>  
>  	pid = fork();
> -	if (pid < 0) {
> +	ASSERT_GE(pid, 0) {
> +		TH_LOG("%s - Failed to fork", strerror(errno));
>  		close_prot_errno_disarm(syncfds[0]);
>  		close_prot_errno_disarm(syncfds[1]);
> -		ksft_exit_fail_msg("%s - Failed to fork", strerror(errno));
>  	}
>  
>  	if (pid == 0) {
>  		int i, j, k, nthreads;
>  		pthread_attr_t attr;
>  		pthread_t threads[DEFAULT_THREADS];
> -		change_userns(syncfds);
> -		change_mountns();
> +		change_userns(_metadata, syncfds);
> +		change_mountns(_metadata);
>  
> -		if (!mkdtemp(binderfs_mntpt))
> -			log_exit("%s - Failed to create binderfs mountpoint\n",
> -				 strerror(errno));
> +		ASSERT_NE(mkdtemp(binderfs_mntpt), NULL) {
> +			TH_LOG("%s - Failed to create binderfs mountpoint",
> +				strerror(errno));
> +		}
>  
>  		ret = mount(NULL, binderfs_mntpt, "binder", 0, 0);
> -		if (ret < 0)
> -			log_exit("%s - Failed to mount binderfs\n", strerror(errno));
> +		ASSERT_EQ(ret, 0) {
> +			TH_LOG("%s - Failed to mount binderfs", strerror(errno));
> +		}
>  
>  		for (int i = 0; i < ARRAY_SIZE(fds); i++) {
>  
>  			snprintf(device_path, sizeof(device_path),
>  				 "%s/binder-control", binderfs_mntpt);
>  			fd = open(device_path, O_RDONLY | O_CLOEXEC);
> -			if (fd < 0)
> -				log_exit("%s - Failed to open binder-control device\n", strerror(errno));
> +			ASSERT_GE(fd, 0) {
> +				TH_LOG("%s - Failed to open binder-control device",
> +					strerror(errno));
> +			}
>  
>  			memset(&device, 0, sizeof(device));
>  			snprintf(device.name, sizeof(device.name), "%d", i);
>  			ret = ioctl(fd, BINDER_CTL_ADD, &device);
>  			close_prot_errno_disarm(fd);
> -			if (ret < 0)
> -				log_exit("%s - Failed to allocate new binder device\n", strerror(errno));
> +			ASSERT_EQ(ret, 0) {
> +				TH_LOG("%s - Failed to allocate new binder device",
> +					strerror(errno));
> +			}
>  
>  			snprintf(device_path, sizeof(device_path), "%s/%d",
>  				 binderfs_mntpt, i);
>  			fds[i] = open(device_path, O_RDONLY | O_CLOEXEC);
> -			if (fds[i] < 0)
> -				log_exit("%s - Failed to open binder device\n", strerror(errno));
> +			ASSERT_GE(fds[i], 0) {
> +				TH_LOG("%s - Failed to open binder device", strerror(errno));
> +			}
>  		}
>  
>  		ret = umount2(binderfs_mntpt, MNT_DETACH);
> -		rmdir_protect_errno(binderfs_mntpt);
> -		if (ret < 0)
> -			log_exit("%s - Failed to unmount binderfs\n", strerror(errno));
> +		ASSERT_EQ(ret, 0) {
> +			TH_LOG("%s - Failed to unmount binderfs", strerror(errno));
> +			rmdir(binderfs_mntpt);
> +		}
>  
>  		nthreads = get_nprocs_conf();
>  		if (nthreads > DEFAULT_THREADS)
>  			nthreads = DEFAULT_THREADS;
>  
> +		_thread_metadata = _metadata;
>  		pthread_attr_init(&attr);
>  		for (k = 0; k < ARRAY_SIZE(fds); k++) {
>  			for (i = 0; i < nthreads; i++) {
>  				ret = pthread_create(&threads[i], &attr, binder_version_thread, INT_TO_PTR(fds[k]));
>  				if (ret) {
> -					ksft_print_msg("%s - Failed to create thread %d\n", strerror(errno), i);
> +					TH_LOG("%s - Failed to create thread %d",
> +						strerror(errno), i);
>  					break;
>  				}
>  			}
> @@ -448,7 +452,8 @@ TEST(binderfs_stress)
>  
>  				ret = pthread_join(threads[j], &fdptr);
>  				if (ret)
> -					ksft_print_msg("%s - Failed to join thread %d for fd %d\n", strerror(errno), j, PTR_TO_INT(fdptr));
> +					TH_LOG("%s - Failed to join thread %d for fd %d",
> +						strerror(errno), j, PTR_TO_INT(fdptr));
>  			}
>  		}
>  		pthread_attr_destroy(&attr);
> @@ -459,11 +464,12 @@ TEST(binderfs_stress)
>  		exit(EXIT_SUCCESS);
>  	}
>  
> -	change_idmaps(syncfds, pid);
> +	change_idmaps(_metadata, syncfds, pid);
>  
>  	ret = wait_for_pid(pid);
> -	if (ret)
> -		ksft_exit_fail_msg("wait_for_pid() failed");
> +	ASSERT_EQ(ret, 0) {
> +		TH_LOG("wait_for_pid() failed");
> +	}
>  }
>  
>  TEST(binderfs_test_privileged)
> @@ -471,7 +477,7 @@ TEST(binderfs_test_privileged)
>  	if (geteuid() != 0)
>  		XFAIL(return, "Tests are not run as root. Skipping privileged tests");
>  
> -	if (__do_binderfs_test() == 1)
> +	if (__do_binderfs_test(_metadata))
>  		XFAIL(return, "The Android binderfs filesystem is not available");
>  }
>  
> @@ -482,31 +488,33 @@ TEST(binderfs_test_unprivileged)
>  	pid_t pid;
>  
>  	ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
> -	if (ret < 0)
> -		ksft_exit_fail_msg("%s - Failed to create socket pair", strerror(errno));
> +	ASSERT_EQ(ret, 0) {
> +		TH_LOG("%s - Failed to create socket pair", strerror(errno));
> +	}
>  
>  	pid = fork();
> -	if (pid < 0) {
> +	ASSERT_GE(pid, 0) {
>  		close_prot_errno_disarm(syncfds[0]);
>  		close_prot_errno_disarm(syncfds[1]);
> -		ksft_exit_fail_msg("%s - Failed to fork", strerror(errno));
> +		TH_LOG("%s - Failed to fork", strerror(errno));
>  	}
>  
>  	if (pid == 0) {
> -		change_userns(syncfds);
> -		if (__do_binderfs_test() == 1)
> +		change_userns(_metadata, syncfds);
> +		if (__do_binderfs_test(_metadata))
>  			exit(2);
>  		exit(EXIT_SUCCESS);
>  	}
>  
> -	change_idmaps(syncfds, pid);
> +	change_idmaps(_metadata, syncfds, pid);
>  
>  	ret = wait_for_pid(pid);
>  	if (ret) {
>  		if (ret == 2)
>  			XFAIL(return, "The Android binderfs filesystem is not available");
> -		else
> -			ksft_exit_fail_msg("wait_for_pid() failed");
> +		ASSERT_EQ(ret, 0) {
> +			TH_LOG("wait_for_pid() failed");
> +		}
>  	}
>  }
>  
> -- 
> 2.25.1
> 

Re: [PATCH 3/7] selftests/binderfs: Fix harness API usage

[Kees Cook]

On Fri, Jun 12, 2020 at 08:19:00PM +0200, Christian Brauner wrote:
> On Thu, Jun 11, 2020 at 03:40:24PM -0700, Kees Cook wrote:
> > The binderfs test mixed the full harness API and the selftest API.
> > Adjust to use only the harness API so that the harness API can switch
> > to using the selftest API internally in future patches.
> > 
> > Cc: Shuah Khan <shuah@kernel.org>
> > Cc: Christian Brauner <christian.brauner@ubuntu.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: linux-kselftest@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> 
> I had this on my TODO but never actually got around to it, so thanks!
> 
> In all honesty, I've done a "Does this overall look sane?" review.
> Simply because I lack the time to do this in more detail right now but
> I'm happy this work is done and so overall:
> 
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

I did actually do runtime tests of this (and found a couple bugs in my
port that I fixed before sending the final version). So, fwiw, it passes
for me.

Thanks for the review!

-Kees

-- 
Kees Cook

[PATCH 4/7] selftests: Add header documentation and helpers

[Kees Cook]

Add "how to use this API" documentation to kselftest.h, and include some
addition helpers and notes to make things easier to use.

Additionally removes the incorrect "Bail out!" line from the standard exit
path. The TAP13 specification says that "Bail out!"  should be used when
giving up before all tests have been run. For a "normal" execution run,
the selftests should not report "Bail out!".

Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/kselftest.h | 60 ++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index 0ac49d91a260..5716cbb9eecc 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -6,6 +6,36 @@
  * Copyright (c) 2014 Shuah Khan <shuahkh@osg.samsung.com>
  * Copyright (c) 2014 Samsung Electronics Co., Ltd.
  *
+ * Using this API consists of first counting how many tests your code
+ * has to run, and then starting up the reporting:
+ *
+ *     ksft_print_header();
+ *     ksft_set_plan(total_number_of_tests);
+ *
+ * For each test, report any progress, debugging, etc with:
+ *
+ *     ksft_print_msg(fmt, ...);
+ *
+ * and finally report the pass/fail/skip state of the test with one of:
+ *
+ *     ksft_test_result(condition, fmt, ...);
+ *     ksft_test_result_pass(fmt, ...);
+ *     ksft_test_result_fail(fmt, ...);
+ *     ksft_test_result_skip(fmt, ...);
+ *     ksft_test_result_error(fmt, ...);
+ *
+ * When all tests are finished, clean up and exit the program with one of:
+ *
+ *    ksft_exit(condition);
+ *    ksft_exit_pass();
+ *    ksft_exit_fail();
+ *
+ * If the program wants to report details on why the entire program has
+ * failed, it can instead exit with a message (this is usually done when
+ * the program is aborting before finishing all tests):
+ *
+ *    ksft_exit_fail_msg(fmt, ...);
+ *
  */
 #ifndef __KSELFTEST_H
 #define __KSELFTEST_H
@@ -74,7 +104,7 @@ static inline void ksft_print_cnts(void)
 	if (ksft_plan != ksft_test_num())
 		printf("# Planned tests != run tests (%u != %u)\n",
 			ksft_plan, ksft_test_num());
-	printf("# Pass %d Fail %d Xfail %d Xpass %d Skip %d Error %d\n",
+	printf("# Totals: pass:%d fail:%d xfail:%d xpass:%d skip:%d error:%d\n",
 		ksft_cnt.ksft_pass, ksft_cnt.ksft_fail,
 		ksft_cnt.ksft_xfail, ksft_cnt.ksft_xpass,
 		ksft_cnt.ksft_xskip, ksft_cnt.ksft_error);
@@ -120,6 +150,20 @@ static inline void ksft_test_result_fail(const char *msg, ...)
 	va_end(args);
 }
 
+/**
+ * ksft_test_result() - Report test success based on truth of condition
+ *
+ * @condition: if true, report test success, otherwise failure.
+ */
+#define ksft_test_result(condition, fmt, ...) do {	\
+	if (!!(condition))				\
+		ksft_test_result_pass(fmt, ##__VA_ARGS__);\
+	else						\
+		ksft_test_result_fail(fmt, ##__VA_ARGS__);\
+	} while (0)
+
+/* TODO: add ksft_test_result_xfail() */
+
 static inline void ksft_test_result_skip(const char *msg, ...)
 {
 	int saved_errno = errno;
@@ -134,6 +178,7 @@ static inline void ksft_test_result_skip(const char *msg, ...)
 	va_end(args);
 }
 
+/* TODO: how does "error" differ from "fail" or "skip"? */
 static inline void ksft_test_result_error(const char *msg, ...)
 {
 	int saved_errno = errno;
@@ -156,11 +201,22 @@ static inline int ksft_exit_pass(void)
 
 static inline int ksft_exit_fail(void)
 {
-	printf("Bail out!\n");
 	ksft_print_cnts();
 	exit(KSFT_FAIL);
 }
 
+/**
+ * ksft_exit() - Exit selftest based on truth of condition
+ *
+ * @condition: if true, exit self test with success, otherwise fail.
+ */
+#define ksft_exit(condition) do {	\
+	if (!!(condition))		\
+		ksft_exit_pass();	\
+	else				\
+		ksft_exit_fail();	\
+	} while (0)
+
 static inline int ksft_exit_fail_msg(const char *msg, ...)
 {
 	int saved_errno = errno;
-- 
2.25.1

[PATCH 5/7] selftests/harness: Switch to TAP output

[Kees Cook]

Using the kselftest_harness.h would result in non-TAP test reporting,
which didn't make much sense given that all the requirements for using
the low-level API were met. Switch to using ksft_*() helpers while
retaining as much of a human-readability as possible.

Cc: Shuah Khan <shuah@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/kselftest.h         |  5 +-
 tools/testing/selftests/kselftest_harness.h | 52 ++++++++++++---------
 2 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index 5716cbb9eecc..3f0d236ca2e4 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -1,7 +1,8 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /*
- * kselftest.h:	kselftest framework return codes to include from
- *		selftests.
+ * kselftest.h:	low-level kselftest framework to include from
+ *		selftest programs. When possible, please use
+ *		kselftest_harness.h instead.
  *
  * Copyright (c) 2014 Shuah Khan <shuahkh@osg.samsung.com>
  * Copyright (c) 2014 Samsung Electronics Co., Ltd.
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index c9f03ef93338..f8f7e47c739a 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -50,7 +50,9 @@
 #ifndef __KSELFTEST_HARNESS_H
 #define __KSELFTEST_HARNESS_H
 
+#ifndef _GNU_SOURCE
 #define _GNU_SOURCE
+#endif
 #include <asm/types.h>
 #include <errno.h>
 #include <stdbool.h>
@@ -62,6 +64,8 @@
 #include <sys/wait.h>
 #include <unistd.h>
 
+#include "kselftest.h"
+
 #define TEST_TIMEOUT_DEFAULT 30
 
 /* Utilities exposed to the test definitions */
@@ -104,7 +108,7 @@
 
 /* Unconditional logger for internal use. */
 #define __TH_LOG(fmt, ...) \
-		fprintf(TH_LOG_STREAM, "%s:%d:%s:" fmt "\n", \
+		fprintf(TH_LOG_STREAM, "# %s:%d:%s:" fmt "\n", \
 			__FILE__, __LINE__, _metadata->name, ##__VA_ARGS__)
 
 /**
@@ -119,7 +123,7 @@
  */
 #define XFAIL(statement, fmt, ...) do { \
 	if (TH_LOG_ENABLED) { \
-		fprintf(TH_LOG_STREAM, "[  XFAIL!  ] " fmt "\n", \
+		fprintf(TH_LOG_STREAM, "#      XFAIL     " fmt "\n", \
 			##__VA_ARGS__); \
 	} \
 	/* TODO: find a way to pass xfail to test runner process. */ \
@@ -813,12 +817,12 @@ static void __timeout_handler(int sig, siginfo_t *info, void *ucontext)
 	/* Sanity check handler execution environment. */
 	if (!t) {
 		fprintf(TH_LOG_STREAM,
-			"no active test in SIGALRM handler!?\n");
+			"# no active test in SIGALRM handler!?\n");
 		abort();
 	}
 	if (sig != SIGALRM || sig != info->si_signo) {
 		fprintf(TH_LOG_STREAM,
-			"%s: SIGALRM handler caught signal %d!?\n",
+			"# %s: SIGALRM handler caught signal %d!?\n",
 			t->name, sig != SIGALRM ? sig : info->si_signo);
 		abort();
 	}
@@ -839,7 +843,7 @@ void __wait_for_test(struct __test_metadata *t)
 	if (sigaction(SIGALRM, &action, &saved_action)) {
 		t->passed = 0;
 		fprintf(TH_LOG_STREAM,
-			"%s: unable to install SIGALRM handler\n",
+			"# %s: unable to install SIGALRM handler\n",
 			t->name);
 		return;
 	}
@@ -851,7 +855,7 @@ void __wait_for_test(struct __test_metadata *t)
 	if (sigaction(SIGALRM, &saved_action, NULL)) {
 		t->passed = 0;
 		fprintf(TH_LOG_STREAM,
-			"%s: unable to uninstall SIGALRM handler\n",
+			"# %s: unable to uninstall SIGALRM handler\n",
 			t->name);
 		return;
 	}
@@ -860,18 +864,17 @@ void __wait_for_test(struct __test_metadata *t)
 	if (t->timed_out) {
 		t->passed = 0;
 		fprintf(TH_LOG_STREAM,
-			"%s: Test terminated by timeout\n", t->name);
+			"# %s: Test terminated by timeout\n", t->name);
 	} else if (WIFEXITED(status)) {
 		t->passed = t->termsig == -1 ? !WEXITSTATUS(status) : 0;
 		if (t->termsig != -1) {
 			fprintf(TH_LOG_STREAM,
-				"%s: Test exited normally "
-				"instead of by signal (code: %d)\n",
+				"# %s: Test exited normally instead of by signal (code: %d)\n",
 				t->name,
 				WEXITSTATUS(status));
 		} else if (!t->passed) {
 			fprintf(TH_LOG_STREAM,
-				"%s: Test failed at step #%d\n",
+				"# %s: Test failed at step #%d\n",
 				t->name,
 				WEXITSTATUS(status));
 		}
@@ -879,20 +882,19 @@ void __wait_for_test(struct __test_metadata *t)
 		t->passed = 0;
 		if (WTERMSIG(status) == SIGABRT) {
 			fprintf(TH_LOG_STREAM,
-				"%s: Test terminated by assertion\n",
+				"# %s: Test terminated by assertion\n",
 				t->name);
 		} else if (WTERMSIG(status) == t->termsig) {
 			t->passed = 1;
 		} else {
 			fprintf(TH_LOG_STREAM,
-				"%s: Test terminated unexpectedly "
-				"by signal %d\n",
+				"# %s: Test terminated unexpectedly by signal %d\n",
 				t->name,
 				WTERMSIG(status));
 		}
 	} else {
 		fprintf(TH_LOG_STREAM,
-			"%s: Test ended in some other way [%u]\n",
+			"# %s: Test ended in some other way [%u]\n",
 			t->name,
 			status);
 	}
@@ -908,11 +910,11 @@ void __run_test(struct __fixture_metadata *f,
 	t->step = 0;
 	t->no_print = 0;
 
-	printf("[ RUN      ] %s%s%s.%s\n",
+	ksft_print_msg(" RUN           %s%s%s.%s ...\n",
 	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
 	t->pid = fork();
 	if (t->pid < 0) {
-		printf("ERROR SPAWNING TEST CHILD\n");
+		ksft_print_msg("ERROR SPAWNING TEST CHILD\n");
 		t->passed = 0;
 	} else if (t->pid == 0) {
 		t->fn(t, variant);
@@ -921,7 +923,9 @@ void __run_test(struct __fixture_metadata *f,
 	} else {
 		__wait_for_test(t);
 	}
-	printf("[     %4s ] %s%s%s.%s\n", (t->passed ? "OK" : "FAIL"),
+	ksft_print_msg("         %4s  %s%s%s.%s\n", t->passed ? "OK" : "FAIL",
+	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
+	ksft_test_result(t->passed, "%s%s%s.%s\n",
 	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
 }
 
@@ -945,8 +949,9 @@ static int test_harness_run(int __attribute__((unused)) argc,
 		}
 	}
 
-	/* TODO(wad) add optional arguments similar to gtest. */
-	printf("[==========] Running %u tests from %u test cases.\n",
+	ksft_print_header();
+	ksft_set_plan(test_count);
+	ksft_print_msg("Starting %u tests from %u test cases.\n",
 	       test_count, case_count);
 	for (f = __fixture_list; f; f = f->next) {
 		for (v = f->variant ?: &no_variant; v; v = v->next) {
@@ -960,9 +965,12 @@ static int test_harness_run(int __attribute__((unused)) argc,
 			}
 		}
 	}
-	printf("[==========] %u / %u tests passed.\n", pass_count, count);
-	printf("[  %s  ]\n", (ret ? "FAILED" : "PASSED"));
-	return ret;
+	ksft_print_msg("%s: %u / %u tests passed.\n", ret ? "FAILED" : "PASSED",
+			pass_count, count);
+	ksft_exit(ret == 0);
+
+	/* unreachable */
+	return KSFT_FAIL;
 }
 
 static void __attribute__((constructor)) __constructor_order_first(void)
-- 
2.25.1

[PATCH 6/7] selftests/harness: Fully track XFAIL reports

[Kees Cook]

Plumb XFAIL conditions up into TAP reporting and test counts.

Cc: Shuah Khan <shuah@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/kselftest.h         | 17 ++++++-
 tools/testing/selftests/kselftest_harness.h | 54 ++++++++++++++++-----
 2 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index 3f0d236ca2e4..9b4efdbb07f6 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -17,12 +17,13 @@
  *
  *     ksft_print_msg(fmt, ...);
  *
- * and finally report the pass/fail/skip state of the test with one of:
+ * and finally report the pass/fail/skip/xfail state of the test with one of:
  *
  *     ksft_test_result(condition, fmt, ...);
  *     ksft_test_result_pass(fmt, ...);
  *     ksft_test_result_fail(fmt, ...);
  *     ksft_test_result_skip(fmt, ...);
+ *     ksft_test_result_xfail(fmt, ...);
  *     ksft_test_result_error(fmt, ...);
  *
  * When all tests are finished, clean up and exit the program with one of:
@@ -163,7 +164,19 @@ static inline void ksft_test_result_fail(const char *msg, ...)
 		ksft_test_result_fail(fmt, ##__VA_ARGS__);\
 	} while (0)
 
-/* TODO: add ksft_test_result_xfail() */
+static inline void ksft_test_result_xfail(const char *msg, ...)
+{
+	int saved_errno = errno;
+	va_list args;
+
+	ksft_cnt.ksft_xfail++;
+
+	va_start(args, msg);
+	printf("ok %d # XFAIL ", ksft_test_num());
+	errno = saved_errno;
+	vprintf(msg, args);
+	va_end(args);
+}
 
 static inline void ksft_test_result_skip(const char *msg, ...)
 {
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index f8f7e47c739a..6b06930468e5 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -126,8 +126,8 @@
 		fprintf(TH_LOG_STREAM, "#      XFAIL     " fmt "\n", \
 			##__VA_ARGS__); \
 	} \
-	/* TODO: find a way to pass xfail to test runner process. */ \
 	_metadata->passed = 1; \
+	_metadata->xfail = 1; \
 	_metadata->trigger = 0; \
 	statement; \
 } while (0)
@@ -777,6 +777,7 @@ struct __test_metadata {
 	struct __fixture_metadata *fixture;
 	int termsig;
 	int passed;
+	int xfail;	/* did XFAIL get used? */
 	int trigger; /* extra handler after the evaluation */
 	int timeout;	/* seconds to wait for test timeout */
 	bool timed_out;	/* did this test timeout instead of exiting? */
@@ -866,17 +867,31 @@ void __wait_for_test(struct __test_metadata *t)
 		fprintf(TH_LOG_STREAM,
 			"# %s: Test terminated by timeout\n", t->name);
 	} else if (WIFEXITED(status)) {
-		t->passed = t->termsig == -1 ? !WEXITSTATUS(status) : 0;
 		if (t->termsig != -1) {
+			t->passed = 0;
 			fprintf(TH_LOG_STREAM,
 				"# %s: Test exited normally instead of by signal (code: %d)\n",
 				t->name,
 				WEXITSTATUS(status));
-		} else if (!t->passed) {
-			fprintf(TH_LOG_STREAM,
-				"# %s: Test failed at step #%d\n",
-				t->name,
-				WEXITSTATUS(status));
+		} else {
+			switch (WEXITSTATUS(status)) {
+			/* Success */
+			case 0:
+				t->passed = 1;
+				break;
+			/* XFAIL */
+			case 255:
+				t->passed = 1;
+				t->xfail = 1;
+				break;
+			/* Other failure, assume step report. */
+			default:
+				t->passed = 0;
+				fprintf(TH_LOG_STREAM,
+					"# %s: Test failed at step #%d\n",
+					t->name,
+					WEXITSTATUS(status));
+			}
 		}
 	} else if (WIFSIGNALED(status)) {
 		t->passed = 0;
@@ -906,6 +921,7 @@ void __run_test(struct __fixture_metadata *f,
 {
 	/* reset test struct */
 	t->passed = 1;
+	t->xfail = 0;
 	t->trigger = 0;
 	t->step = 0;
 	t->no_print = 0;
@@ -918,15 +934,31 @@ void __run_test(struct __fixture_metadata *f,
 		t->passed = 0;
 	} else if (t->pid == 0) {
 		t->fn(t, variant);
-		/* return the step that failed or 0 */
-		_exit(t->passed ? 0 : t->step);
+		/* Make sure step doesn't get lost in reporting */
+		if (t->step >= 255) {
+			ksft_print_msg("Too many test steps (%u)!?\n", t->step);
+			t->step = 254;
+		}
+		/* Use 255 for XFAIL */
+		if (t->xfail)
+			_exit(255);
+		/* Pass is exit 0 */
+		if (t->passed)
+			_exit(0);
+		/* Something else happened, report the step. */
+		_exit(t->step);
 	} else {
 		__wait_for_test(t);
 	}
 	ksft_print_msg("         %4s  %s%s%s.%s\n", t->passed ? "OK" : "FAIL",
 	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
-	ksft_test_result(t->passed, "%s%s%s.%s\n",
-	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
+
+	if (t->xfail)
+		ksft_test_result_xfail("%s%s%s.%s\n",
+			f->name, variant->name[0] ? "." : "", variant->name, t->name);
+	else
+		ksft_test_result(t->passed, "%s%s%s.%s\n",
+			f->name, variant->name[0] ? "." : "", variant->name, t->name);
 }
 
 static int test_harness_run(int __attribute__((unused)) argc,
-- 
2.25.1

[PATCH 7/7] selftests/harness: Display signed values correctly

[Kees Cook]

Since forever the harness output for signed value tests have reported
unsigned values to avoid casting. Instead, actually test the variable
types and perform the correct casts and choose the correct format
specifiers.

Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/kselftest_harness.h | 42 ++++++++++++++++++---
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 6b06930468e5..8802f20d1915 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -679,17 +679,49 @@
 	if (_metadata->passed && _metadata->step < 255) \
 		_metadata->step++;
 
+#define is_signed_type(var)       (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
+
 #define __EXPECT(_expected, _expected_str, _seen, _seen_str, _t, _assert) do { \
 	/* Avoid multiple evaluation of the cases */ \
 	__typeof__(_expected) __exp = (_expected); \
 	__typeof__(_seen) __seen = (_seen); \
 	if (_assert) __INC_STEP(_metadata); \
 	if (!(__exp _t __seen)) { \
-		unsigned long long __exp_print = (uintptr_t)__exp; \
-		unsigned long long __seen_print = (uintptr_t)__seen; \
-		__TH_LOG("Expected %s (%llu) %s %s (%llu)", \
-			 _expected_str, __exp_print, #_t, \
-			 _seen_str, __seen_print); \
+		/* Report with actual signedness to avoid weird output. */ \
+		switch (is_signed_type(__exp) * 2 + is_signed_type(__seen)) { \
+		case 0: { \
+			unsigned long long __exp_print = (uintptr_t)__exp; \
+			unsigned long long __seen_print = (uintptr_t)__seen; \
+			__TH_LOG("Expected %s (%llu) %s %s (%llu)", \
+				 _expected_str, __exp_print, #_t, \
+				 _seen_str, __seen_print); \
+			break; \
+			} \
+		case 1: { \
+			unsigned long long __exp_print = (uintptr_t)__exp; \
+			long long __seen_print = (intptr_t)__seen; \
+			__TH_LOG("Expected %s (%llu) %s %s (%lld)", \
+				 _expected_str, __exp_print, #_t, \
+				 _seen_str, __seen_print); \
+			break; \
+			} \
+		case 2: { \
+			long long __exp_print = (intptr_t)__exp; \
+			unsigned long long __seen_print = (uintptr_t)__seen; \
+			__TH_LOG("Expected %s (%lld) %s %s (%llu)", \
+				 _expected_str, __exp_print, #_t, \
+				 _seen_str, __seen_print); \
+			break; \
+			} \
+		case 3: { \
+			long long __exp_print = (intptr_t)__exp; \
+			long long __seen_print = (intptr_t)__seen; \
+			__TH_LOG("Expected %s (%lld) %s %s (%lld)", \
+				 _expected_str, __exp_print, #_t, \
+				 _seen_str, __seen_print); \
+			break; \
+			} \
+		} \
 		_metadata->passed = 0; \
 		/* Ensure the optional handler is triggered */ \
 		_metadata->trigger = 1; \
-- 
2.25.1