diff mbox series

[5/5] selftest/seccomp: add a new test for the sync mode of seccomp_user_notify

Message ID 20221020011048.156415-6-avagin@gmail.com
State New
Headers
Series seccomp: add the synchronous mode for seccomp_unotify |

Commit Message

Andrei Vagin Oct. 20, 2022, 1:10 a.m. UTC 
  Test output:
RUN           global.user_notification_sync ...
seccomp_bpf.c:4279:user_notification_sync:basic: 8655 nsec/syscall
seccomp_bpf.c:4279:user_notification_sync:sync:	 2919 nsec/syscall
OK  global.user_notification_sync

Signed-off-by: Andrei Vagin <avagin@gmail.com>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)

Comments

Kees Cook Oct. 20, 2022, 5:04 a.m. UTC | #1 
On October 19, 2022 6:10:48 PM PDT, Andrei Vagin <avagin@gmail.com> wrote:
>Test output:
>RUN           global.user_notification_sync ...
>seccomp_bpf.c:4279:user_notification_sync:basic: 8655 nsec/syscall
>seccomp_bpf.c:4279:user_notification_sync:sync:	 2919 nsec/syscall
>OK  global.user_notification_sync

This looks like a benchmark, not a functionality test. But maybe the test is "is sync faster than async?"

>
>Signed-off-by: Andrei Vagin <avagin@gmail.com>
>---
> tools/testing/selftests/seccomp/seccomp_bpf.c | 80 +++++++++++++++++++
> 1 file changed, 80 insertions(+)
>
>diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
>index 4ae6c8991307..01f872415c17 100644
>--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
>+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
>@@ -4241,6 +4241,86 @@ TEST(user_notification_addfd_rlimit)
> 	close(memfd);
> }
> 
>+/* USER_NOTIF_BENCH_TIMEOUT is 100 miliseconds. */
>+#define USER_NOTIF_BENCH_TIMEOUT  100000000ULL
>+#define NSECS_PER_SEC            1000000000ULL
>+
>+#ifndef SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP
>+#define SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP (1UL << 0)
>+#define SECCOMP_IOCTL_NOTIF_SET_FLAGS  SECCOMP_IOW(4, __u64)
>+#endif
>+
>+static void user_notification_sync_loop(struct __test_metadata *_metadata,
>+					char *test_name, int listener)
>+{
>+	struct timespec ts;
>+	uint64_t start, end, nr;
>+	struct seccomp_notif req = {};
>+	struct seccomp_notif_resp resp = {};
>+
>+	clock_gettime(CLOCK_MONOTONIC, &ts);
>+	start = ts.tv_nsec + ts.tv_sec * NSECS_PER_SEC;
>+	for (end = start, nr = 0; end - start < USER_NOTIF_BENCH_TIMEOUT; nr++) {
>+		memset(&req, 0, sizeof(req));
>+		req.pid = 0;
>+		EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
>+
>+		EXPECT_EQ(req.data.nr,  __NR_getppid);
>+
>+		resp.id = req.id;
>+		resp.error = 0;
>+		resp.val = USER_NOTIF_MAGIC;
>+		resp.flags = 0;
>+		EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);

I think these EXPECTs should be ASSERTs...

>+
>+		clock_gettime(CLOCK_MONOTONIC, &ts);
>+		end = ts.tv_nsec + ts.tv_sec * NSECS_PER_SEC;
>+	}
>+	TH_LOG("%s:\t%lld nsec/syscall", test_name, USER_NOTIF_BENCH_TIMEOUT / nr);
>+}
>+
>+TEST(user_notification_sync)
>+{
>+	pid_t pid;
>+	long ret;
>+	int status, listener;
>+
>+	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
>+	ASSERT_EQ(0, ret) {
>+		TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
>+	}
>+
>+	listener = user_notif_syscall(__NR_getppid,
>+				      SECCOMP_FILTER_FLAG_NEW_LISTENER);
>+	ASSERT_GE(listener, 0);
>+
>+	pid = fork();
>+	ASSERT_GE(pid, 0);
>+
>+	if (pid == 0) {
>+		while (1) {
>+			ret = syscall(__NR_getppid);
>+			if (ret == USER_NOTIF_MAGIC)
>+				continue;
>+			break;
>+		}
>+		_exit(1);
>+	}
>+
>+	user_notification_sync_loop(_metadata, "basic", listener);
>+
>+	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SET_FLAGS,
>+			SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP, 0), 0);

Same here.

Also can you test that invalid SET_FLAGS are correctly rejected here?

>+
>+	user_notification_sync_loop(_metadata, "sync", listener);
>+

If the timings are collected, add a test that sync is <= async here?

>+	kill(pid, SIGKILL);
>+	EXPECT_EQ(waitpid(pid, &status, 0), pid);
>+	EXPECT_EQ(true, WIFSIGNALED(status));
>+	EXPECT_EQ(SIGKILL, WTERMSIG(status));
>+}
>+
>+
> /* Make sure PTRACE_O_SUSPEND_SECCOMP requires CAP_SYS_ADMIN. */
> FIXTURE(O_SUSPEND_SECCOMP) {
> 	pid_t pid;

Otherwise, yeah, looks good.
Andrei Vagin Oct. 21, 2022, 12:53 a.m. UTC | #2 
On Wed, Oct 19, 2022 at 10:04:44PM -0700, Kees Cook wrote:
> On October 19, 2022 6:10:48 PM PDT, Andrei Vagin <avagin@gmail.com> wrote:
> >Test output:
> >RUN           global.user_notification_sync ...
> >seccomp_bpf.c:4279:user_notification_sync:basic: 8655 nsec/syscall
> >seccomp_bpf.c:4279:user_notification_sync:sync:	 2919 nsec/syscall
> >OK  global.user_notification_sync
> 
> This looks like a benchmark, not a functionality test. But maybe the test is "is sync faster than async?"
> 

Yes, it is. I found it quite useful for debugging and understanding that
everything works as expected. I like the idea to check that sync is
faster than async. I will add it and address all your other comments in
the next version. Thanks.
diff mbox series

Patch

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 4ae6c8991307..01f872415c17 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -4241,6 +4241,86 @@  TEST(user_notification_addfd_rlimit)
 	close(memfd);
 }
 
+/* USER_NOTIF_BENCH_TIMEOUT is 100 miliseconds. */
+#define USER_NOTIF_BENCH_TIMEOUT  100000000ULL
+#define NSECS_PER_SEC            1000000000ULL
+
+#ifndef SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP
+#define SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP (1UL << 0)
+#define SECCOMP_IOCTL_NOTIF_SET_FLAGS  SECCOMP_IOW(4, __u64)
+#endif
+
+static void user_notification_sync_loop(struct __test_metadata *_metadata,
+					char *test_name, int listener)
+{
+	struct timespec ts;
+	uint64_t start, end, nr;
+	struct seccomp_notif req = {};
+	struct seccomp_notif_resp resp = {};
+
+	clock_gettime(CLOCK_MONOTONIC, &ts);
+	start = ts.tv_nsec + ts.tv_sec * NSECS_PER_SEC;
+	for (end = start, nr = 0; end - start < USER_NOTIF_BENCH_TIMEOUT; nr++) {
+		memset(&req, 0, sizeof(req));
+		req.pid = 0;
+		EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
+
+		EXPECT_EQ(req.data.nr,  __NR_getppid);
+
+		resp.id = req.id;
+		resp.error = 0;
+		resp.val = USER_NOTIF_MAGIC;
+		resp.flags = 0;
+		EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
+
+		clock_gettime(CLOCK_MONOTONIC, &ts);
+		end = ts.tv_nsec + ts.tv_sec * NSECS_PER_SEC;
+	}
+	TH_LOG("%s:\t%lld nsec/syscall", test_name, USER_NOTIF_BENCH_TIMEOUT / nr);
+}
+
+TEST(user_notification_sync)
+{
+	pid_t pid;
+	long ret;
+	int status, listener;
+
+	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+	ASSERT_EQ(0, ret) {
+		TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
+	}
+
+	listener = user_notif_syscall(__NR_getppid,
+				      SECCOMP_FILTER_FLAG_NEW_LISTENER);
+	ASSERT_GE(listener, 0);
+
+	pid = fork();
+	ASSERT_GE(pid, 0);
+
+	if (pid == 0) {
+		while (1) {
+			ret = syscall(__NR_getppid);
+			if (ret == USER_NOTIF_MAGIC)
+				continue;
+			break;
+		}
+		_exit(1);
+	}
+
+	user_notification_sync_loop(_metadata, "basic", listener);
+
+	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SET_FLAGS,
+			SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP, 0), 0);
+
+	user_notification_sync_loop(_metadata, "sync", listener);
+
+	kill(pid, SIGKILL);
+	EXPECT_EQ(waitpid(pid, &status, 0), pid);
+	EXPECT_EQ(true, WIFSIGNALED(status));
+	EXPECT_EQ(SIGKILL, WTERMSIG(status));
+}
+
+
 /* Make sure PTRACE_O_SUSPEND_SECCOMP requires CAP_SYS_ADMIN. */
 FIXTURE(O_SUSPEND_SECCOMP) {
 	pid_t pid;