Hi, Here are a few fixes for seccomp_bpf tests found when testing on Android:
user_notification_sibling_pid_ns: unshare(CLONE_NEWPID) can return EINVAL so have added a check for this.
KILL_THREAD: This one is a bit more Android specific. In Bionic pthread_create is calling prctl, this is causing the test to fail as prctl is in the filter for this test and is killed when it is called. I've just changed prctl to getpid in this case.
user_notification_addfd: This test can fail if there are existing file descriptors when the test starts. It expects the next file descriptor to always increase sequentially which is not always the case. Added a get_next_fd function to return the next expected file descriptor.
Regards,
Terry
Terry Tritton (3): selftests/seccomp: Handle EINVAL on unshare(CLONE_NEWPID) selftests/seccomp: Change the syscall used in KILL_THREAD test selftests/seccomp: user_notification_addfd check nextfd is available
tools/testing/selftests/seccomp/seccomp_bpf.c | 41 ++++++++++++++----- 1 file changed, 31 insertions(+), 10 deletions(-)
unshare(CLONE_NEWPID) can return EINVAL if the kernel does not have the CONFIG_PID_NS option enabled.
Add a check on these calls to skip the test if we receive EINVAL.
Signed-off-by: Terry Tritton terry.tritton@linaro.org --- tools/testing/selftests/seccomp/seccomp_bpf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 38f651469968..5e705674b706 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -3709,7 +3709,12 @@ TEST(user_notification_sibling_pid_ns) ASSERT_GE(pid, 0);
if (pid == 0) { - ASSERT_EQ(unshare(CLONE_NEWPID), 0); + ASSERT_EQ(unshare(CLONE_NEWPID), 0) { + if (errno == EPERM) + SKIP(return, "CLONE_NEWPID requires CAP_SYS_ADMIN"); + else if (errno == EINVAL) + SKIP(return, "CLONE_NEWPID is invalid (missing CONFIG_PID_NS?)"); + }
pid2 = fork(); ASSERT_GE(pid2, 0); @@ -3727,6 +3732,8 @@ TEST(user_notification_sibling_pid_ns) ASSERT_EQ(unshare(CLONE_NEWPID), 0) { if (errno == EPERM) SKIP(return, "CLONE_NEWPID requires CAP_SYS_ADMIN"); + else if (errno == EINVAL) + SKIP(return, "CLONE_NEWPID is invalid (missing CONFIG_PID_NS?)"); } ASSERT_EQ(errno, 0);
The Bionic version of pthread_create used on Android calls the prctl function to give the stack and thread local storage a useful name. This will cause the KILL_THREAD test to fail as it will kill the thread as soon as it is created.
change the test to use getpid instead of prctl.
Signed-off-by: Terry Tritton terry.tritton@linaro.org --- tools/testing/selftests/seccomp/seccomp_bpf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 5e705674b706..da11b95b8872 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -784,7 +784,7 @@ void *kill_thread(void *data) bool die = (bool)data;
if (die) { - prctl(PR_GET_SECCOMP, 0, 0, 0, 0); + syscall(__NR_getpid); return (void *)SIBLING_EXIT_FAILURE; }
@@ -803,11 +803,11 @@ void kill_thread_or_group(struct __test_metadata *_metadata, { pthread_t thread; void *status; - /* Kill only when calling __NR_prctl. */ + /* Kill only when calling __NR_getpid. */ struct sock_filter filter_thread[] = { BPF_STMT(BPF_LD|BPF_W|BPF_ABS, offsetof(struct seccomp_data, nr)), - BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_prctl, 0, 1), + BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getpid, 0, 1), BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL_THREAD), BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW), }; @@ -819,7 +819,7 @@ void kill_thread_or_group(struct __test_metadata *_metadata, struct sock_filter filter_process[] = { BPF_STMT(BPF_LD|BPF_W|BPF_ABS, offsetof(struct seccomp_data, nr)), - BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_prctl, 0, 1), + BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getpid, 0, 1), BPF_STMT(BPF_RET|BPF_K, kill), BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW), };
Currently the user_notification_addfd test checks what the next expected file descriptor will be by incrementing a variable nextfd. This does not account for file descriptors that may already be open before the test is started and will cause the test to fail if any exist.
Replace nextfd++ with a function get_next_fd which will check and return the next available file descriptor.
Signed-off-by: Terry Tritton terry.tritton@linaro.org --- tools/testing/selftests/seccomp/seccomp_bpf.c | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index da11b95b8872..cacf6507f690 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -4044,6 +4044,16 @@ TEST(user_notification_filter_empty_threaded) EXPECT_GT((pollfd.revents & POLLHUP) ?: 0, 0); }
+ +int get_next_fd(int prev_fd) +{ + for (int i = prev_fd + 1; i < FD_SETSIZE; ++i) { + if (fcntl(i, F_GETFD) == -1) + return i; + } + _exit(EXIT_FAILURE); +} + TEST(user_notification_addfd) { pid_t pid; @@ -4060,7 +4070,7 @@ TEST(user_notification_addfd) /* There may be arbitrary already-open fds at test start. */ memfd = memfd_create("test", 0); ASSERT_GE(memfd, 0); - nextfd = memfd + 1; + nextfd = get_next_fd(memfd);
ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); ASSERT_EQ(0, ret) { @@ -4071,7 +4081,8 @@ TEST(user_notification_addfd) /* Check that the basic notification machinery works */ listener = user_notif_syscall(__NR_getppid, SECCOMP_FILTER_FLAG_NEW_LISTENER); - ASSERT_EQ(listener, nextfd++); + ASSERT_EQ(listener, nextfd); + nextfd = get_next_fd(nextfd);
pid = fork(); ASSERT_GE(pid, 0); @@ -4126,14 +4137,16 @@ TEST(user_notification_addfd)
/* Verify we can set an arbitrary remote fd */ fd = ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd); - EXPECT_EQ(fd, nextfd++); + EXPECT_EQ(fd, nextfd); + nextfd = get_next_fd(nextfd); EXPECT_EQ(filecmp(getpid(), pid, memfd, fd), 0);
/* Verify we can set an arbitrary remote fd with large size */ memset(&big, 0x0, sizeof(big)); big.addfd = addfd; fd = ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD_BIG, &big); - EXPECT_EQ(fd, nextfd++); + EXPECT_EQ(fd, nextfd); + nextfd = get_next_fd(nextfd);
/* Verify we can set a specific remote fd */ addfd.newfd = 42; @@ -4171,7 +4184,8 @@ TEST(user_notification_addfd) * Child has earlier "low" fds and now 42, so we expect the next * lowest available fd to be assigned here. */ - EXPECT_EQ(fd, nextfd++); + EXPECT_EQ(fd, nextfd); + nextfd = get_next_fd(nextfd); ASSERT_EQ(filecmp(getpid(), pid, memfd, fd), 0);
/*
On Wed, 24 Jan 2024 14:13:54 +0000, Terry Tritton wrote:
Here are a few fixes for seccomp_bpf tests found when testing on Android:
user_notification_sibling_pid_ns: unshare(CLONE_NEWPID) can return EINVAL so have added a check for this.
KILL_THREAD: This one is a bit more Android specific. In Bionic pthread_create is calling prctl, this is causing the test to fail as prctl is in the filter for this test and is killed when it is called. I've just changed prctl to getpid in this case.
[...]
Thanks for tracking all of these down. These look good to me.
Applied to for-next/seccomp, thanks!
[1/3] selftests/seccomp: Handle EINVAL on unshare(CLONE_NEWPID) https://git.kernel.org/kees/c/18975ce05799 [2/3] selftests/seccomp: Change the syscall used in KILL_THREAD test https://git.kernel.org/kees/c/fbcdf41167fe [3/3] selftests/seccomp: user_notification_addfd check nextfd is available https://git.kernel.org/kees/c/0c6f28a84431
Take care,
linux-kselftest-mirror@lists.linaro.org