| Message ID | 20220428015447.13661-2-sargun@sargun.me (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 662340ef921828507c931da6db303fa3cb02228e |
| Headers | show |
| Series | None | expand |
On Wed, Apr 27, 2022 at 06:54:47PM -0700, Sargun Dhillon wrote: > + /* Start children, and them generate notifications */ ^^ - they maybe? > + for (i = 0; i < ARRAY_SIZE(pids); i++) { > + pid = fork(); > + if (pid == 0) { > + ret = syscall(__NR_getppid); > + exit(ret != USER_NOTIF_MAGIC); > + } > + pids[i] = pid; > + } > + > + /* This spins until all of the children are sleeping */ > +restart_wait: > + for (i = 0; i < ARRAY_SIZE(pids); i++) { > + if (get_proc_stat(pids[i]) != 'S') { > + nanosleep(&delay, NULL); > + goto restart_wait; > + } > + } I wonder if we should/can combine this loop with the previous one, and wait for the child to sleep in getppid() before we fork the next one. Otherwise isn't racy in the case that your loop continues to the next iteration before the child processes are scheduled, so things might be out of order? Maybe I'm missing something. In any case, this change seems reasonable to me. Tycho
On Thu, Apr 28, 2022 at 6:15 AM Tycho Andersen <tycho@tycho.pizza> wrote: > > On Wed, Apr 27, 2022 at 06:54:47PM -0700, Sargun Dhillon wrote: > > + /* Start children, and them generate notifications */ > ^^ - they maybe? > Whoops, this was supposed to be: /* Start children, and generate notifications */ > > + for (i = 0; i < ARRAY_SIZE(pids); i++) { > > + pid = fork(); > > + if (pid == 0) { > > + ret = syscall(__NR_getppid); > > + exit(ret != USER_NOTIF_MAGIC); > > + } > > + pids[i] = pid; > > + } > > + > > + /* This spins until all of the children are sleeping */ > > +restart_wait: > > + for (i = 0; i < ARRAY_SIZE(pids); i++) { > > + if (get_proc_stat(pids[i]) != 'S') { > > + nanosleep(&delay, NULL); > > + goto restart_wait; > > + } > > + } > > I wonder if we should/can combine this loop with the previous one, and > wait for the child to sleep in getppid() before we fork the next one. > Otherwise isn't racy in the case that your loop continues to the next > iteration before the child processes are scheduled, so things might be > out of order? Maybe I'm missing something. > > In any case, this change seems reasonable to me. > > Tycho It's okay if the child processes are started out of order. The test just verifies that the calls are delivered in FIFO order according to when the syscall was called (not when the process was started), and we do this by just looking at the notification ID. It doesn't care about which process generated the notification.
On Thu, Apr 28, 2022 at 09:38:10AM -0700, Sargun Dhillon wrote: > On Thu, Apr 28, 2022 at 6:15 AM Tycho Andersen <tycho@tycho.pizza> wrote: > > > + for (i = 0; i < ARRAY_SIZE(pids); i++) { > > > + pid = fork(); > > > + if (pid == 0) { > > > + ret = syscall(__NR_getppid); > > > + exit(ret != USER_NOTIF_MAGIC); > > > + } > > > + pids[i] = pid; > > > + } > > > + > > > + /* This spins until all of the children are sleeping */ > > > +restart_wait: > > > + for (i = 0; i < ARRAY_SIZE(pids); i++) { > > > + if (get_proc_stat(pids[i]) != 'S') { > > > + nanosleep(&delay, NULL); > > > + goto restart_wait; > > > + } > > > + } > > > > I wonder if we should/can combine this loop with the previous one, and > > wait for the child to sleep in getppid() before we fork the next one. > > Otherwise isn't racy in the case that your loop continues to the next > > iteration before the child processes are scheduled, so things might be > > out of order? Maybe I'm missing something. > > > > In any case, this change seems reasonable to me. > > > > Tycho > It's okay if the child processes are started out of order. The test just > verifies that the calls are delivered in FIFO order according to when > the syscall was called (not when the process was started), and we do > this by just looking at the notification ID. It doesn't care about which > process generated the notification. I totally missed that you don't this, I just assumed you did. Thanks. Anyway, you can add: Acked-by: Tycho Andersen <tycho@tycho.pizza> to both patches. Tycho
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 9d126d7fabdb..33fb3d0c3347 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -4231,6 +4231,115 @@ TEST(user_notification_addfd_rlimit) close(memfd); } +static char get_proc_stat(int pid) +{ + char proc_path[100] = {0}; + char *line = NULL; + size_t len = 0; + ssize_t nread; + char status; + FILE *f; + int i; + + snprintf(proc_path, sizeof(proc_path), "/proc/%d/stat", pid); + f = fopen(proc_path, "r"); + if (f == NULL) + ksft_exit_fail_msg("%s - Could not open %s\n", + strerror(errno), proc_path); + + for (i = 0; i < 3; i++) { + nread = getdelim(&line, &len, ' ', f); + if (nread <= 0) + ksft_exit_fail_msg("Failed to read status: %s\n", + strerror(errno)); + } + + status = *line; + free(line); + fclose(f); + + return status; +} + +TEST(user_notification_fifo) +{ + struct seccomp_notif_resp resp = {}; + struct seccomp_notif req = {}; + int i, status, listener; + pid_t pid, pids[3]; + __u64 baseid; + long ret; + /* 100 ms */ + struct timespec delay = { .tv_nsec = 100000000 }; + + 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!"); + } + + /* Setup a listener */ + listener = user_notif_syscall(__NR_getppid, + SECCOMP_FILTER_FLAG_NEW_LISTENER); + ASSERT_GE(listener, 0); + + pid = fork(); + ASSERT_GE(pid, 0); + + if (pid == 0) { + ret = syscall(__NR_getppid); + exit(ret != USER_NOTIF_MAGIC); + } + + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0); + baseid = req.id + 1; + + resp.id = req.id; + resp.error = 0; + resp.val = USER_NOTIF_MAGIC; + + /* check that we make sure flags == 0 */ + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0); + + EXPECT_EQ(waitpid(pid, &status, 0), pid); + EXPECT_EQ(true, WIFEXITED(status)); + EXPECT_EQ(0, WEXITSTATUS(status)); + + /* Start children, and them generate notifications */ + for (i = 0; i < ARRAY_SIZE(pids); i++) { + pid = fork(); + if (pid == 0) { + ret = syscall(__NR_getppid); + exit(ret != USER_NOTIF_MAGIC); + } + pids[i] = pid; + } + + /* This spins until all of the children are sleeping */ +restart_wait: + for (i = 0; i < ARRAY_SIZE(pids); i++) { + if (get_proc_stat(pids[i]) != 'S') { + nanosleep(&delay, NULL); + goto restart_wait; + } + } + + /* Read the notifications in order (and respond) */ + for (i = 0; i < ARRAY_SIZE(pids); i++) { + memset(&req, 0, sizeof(req)); + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0); + EXPECT_EQ(req.id, baseid + i); + resp.id = req.id; + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0); + } + + /* Make sure notifications were received */ + for (i = 0; i < ARRAY_SIZE(pids); i++) { + EXPECT_EQ(waitpid(pids[i], &status, 0), pids[i]); + EXPECT_EQ(true, WIFEXITED(status)); + EXPECT_EQ(0, WEXITSTATUS(status)); + } +} + /* * TODO: * - expand NNP testing
When multiple notifications are waiting, ensure they show up in order, as defined by the (predictable) seccomp notification ID. This ensures FIFO ordering of notification delivery as notification ids are monitonic and decided when the notification is generated (as opposed to received). Signed-off-by: Sargun Dhillon <sargun@sargun.me> Cc: linux-kselftest@vger.kernel.org --- tools/testing/selftests/seccomp/seccomp_bpf.c | 109 ++++++++++++++++++ 1 file changed, 109 insertions(+)