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

From: Kees Cook
Date: Thu Oct 20 2022 - 01:05:25 EST


On October 19, 2022 6:10:48 PM PDT, Andrei Vagin <avagin@xxxxxxxxx> 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@xxxxxxxxx>
>---
> 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.


--
Kees Cook