From 1cd904949879cbcff2070aeedf3000ceb47558ef Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Thu, 20 Feb 2025 11:13:51 +0100 Subject: [PATCH 1/5] test-async: Specify FORK_REOPEN_LOG in one more place Otherwise debugging assertions from the child process is rather hard. --- src/test/test-async.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/test-async.c b/src/test/test-async.c index ed5fec74d3..f037bac2bb 100644 --- a/src/test/test-async.c +++ b/src/test/test-async.c @@ -81,7 +81,7 @@ TEST(asynchronous_rm_rf) { /* Do this once more, from a subreaper. Which is nice, because we can watch the async child even * though detached */ - r = safe_fork("(subreaper)", FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_DEATHSIG_SIGTERM|FORK_LOG|FORK_WAIT, NULL); + r = safe_fork("(subreaper)", FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_DEATHSIG_SIGTERM|FORK_REOPEN_LOG|FORK_LOG|FORK_WAIT, NULL); ASSERT_OK(r); if (r == 0) { From 216f876d72c0f2dcf1c91b4e32cf062c53ec6b58 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Thu, 20 Feb 2025 11:14:13 +0100 Subject: [PATCH 2/5] test-async: Use ASSERT_OK_ERRNO() instead of ASSERT_OK() where applicable --- src/test/test-async.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/test-async.c b/src/test/test-async.c index f037bac2bb..443505c3eb 100644 --- a/src/test/test-async.c +++ b/src/test/test-async.c @@ -100,7 +100,7 @@ TEST(asynchronous_rm_rf) { for (;;) { siginfo_t si = {}; - ASSERT_OK(waitid(P_ALL, 0, &si, WEXITED)); + ASSERT_OK_ERRNO(waitid(P_ALL, 0, &si, WEXITED)); if (access(tt, F_OK) < 0) { assert_se(errno == ENOENT); From f48103ea614cc22e4f323dc81208bb8f997949a1 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Thu, 20 Feb 2025 09:24:58 +0100 Subject: [PATCH 3/5] process-util: Implement safe_fork_full() on top of pidref_safe_fork_full() Let's switch things around, and move the internals of safe_fork_full() into pidref_safe_fork_full() and make safe_fork_full() a trivial wrapper on top of pidref_safe_fork_full(). --- src/basic/process-util.c | 63 ++++++++++++++++++++++++++-------------- src/basic/process-util.h | 25 ++++++++-------- 2 files changed, 54 insertions(+), 34 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index fbde9c35e6..6a55cca625 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1525,13 +1525,13 @@ static int fork_flags_to_signal(ForkFlags flags) { SIGKILL; } -int safe_fork_full( +int pidref_safe_fork_full( const char *name, const int stdio_fds[3], int except_fds[], size_t n_except_fds, ForkFlags flags, - pid_t *ret_pid) { + PidRef *ret_pid) { pid_t original_pid, pid; sigset_t saved_ss, ss; @@ -1542,8 +1542,9 @@ int safe_fork_full( assert(!FLAGS_SET(flags, FORK_DETACH) || (!ret_pid && (flags & (FORK_WAIT|FORK_DEATHSIG_SIGTERM|FORK_DEATHSIG_SIGINT|FORK_DEATHSIG_SIGKILL)) == 0)); - /* A wrapper around fork(), that does a couple of important initializations in addition to mere forking. Always - * returns the child's PID in *ret_pid. Returns == 0 in the child, and > 0 in the parent. */ + /* A wrapper around fork(), that does a couple of important initializations in addition to mere + * forking. If provided, ret_pid is initialized in both the parent and the child process, both times + * referencing the child process. Returns == 0 in the child and > 0 in the parent. */ prio = flags & FORK_LOG ? LOG_ERR : LOG_DEBUG; @@ -1630,10 +1631,23 @@ int safe_fork_full( return r; if (r != EXIT_SUCCESS) /* exit status > 0 should be treated as failure, too */ return -EPROTO; + + /* If we are in the parent and successfully waited, then the process doesn't exist anymore. */ + if (ret_pid) + *ret_pid = PIDREF_NULL; + + return 1; } - if (ret_pid) - *ret_pid = pid; + if (ret_pid) { + if (FLAGS_SET(flags, FORK_PID_ONLY)) + *ret_pid = PIDREF_MAKE_FROM_PID(pid); + else { + r = pidref_set_pid(ret_pid, pid); + if (r < 0) /* Let's not fail for this, no matter what, the process exists after all, and that's key */ + *ret_pid = PIDREF_MAKE_FROM_PID(pid); + } + } return 1; } @@ -1801,36 +1815,41 @@ int safe_fork_full( if (FLAGS_SET(flags, FORK_FREEZE)) freeze(); - if (ret_pid) - *ret_pid = getpid_cached(); + if (ret_pid) { + if (FLAGS_SET(flags, FORK_PID_ONLY)) + *ret_pid = PIDREF_MAKE_FROM_PID(getpid_cached()); + else { + r = pidref_set_self(ret_pid); + if (r < 0) { + log_full_errno(prio, r, "Failed to acquire PID reference on ourselves: %m"); + _exit(EXIT_FAILURE); + } + } + } return 0; } -int pidref_safe_fork_full( +int safe_fork_full( const char *name, const int stdio_fds[3], int except_fds[], size_t n_except_fds, ForkFlags flags, - PidRef *ret_pid) { + pid_t *ret_pid) { - pid_t pid; - int r, q; + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; + int r; - r = safe_fork_full(name, stdio_fds, except_fds, n_except_fds, flags, &pid); + /* Getting the detached child process pid without pidfd is racy, so don't allow it if not returning + * a pidref to the caller. */ + assert(!FLAGS_SET(flags, FORK_DETACH) || !ret_pid); + + r = pidref_safe_fork_full(name, stdio_fds, except_fds, n_except_fds, flags|FORK_PID_ONLY, ret_pid ? &pidref : NULL); if (r < 0 || !ret_pid) return r; - if (r > 0 && FLAGS_SET(flags, FORK_WAIT)) { - /* If we are in the parent and successfully waited, then the process doesn't exist anymore */ - *ret_pid = PIDREF_NULL; - return r; - } - - q = pidref_set_pid(ret_pid, pid); - if (q < 0) /* Let's not fail for this, no matter what, the process exists after all, and that's key */ - *ret_pid = PIDREF_MAKE_FROM_PID(pid); + *ret_pid = pidref.pid; return r; } diff --git a/src/basic/process-util.h b/src/basic/process-util.h index 58fff2b174..c9b35d79f0 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -193,20 +193,9 @@ typedef enum ForkFlags { FORK_NEW_NETNS = 1 << 20, /* Run child in its own network namespace 💣 DO NOT USE IN THREADED PROGRAMS! 💣 */ FORK_NEW_PIDNS = 1 << 21, /* Run child in its own PID namespace 💣 DO NOT USE IN THREADED PROGRAMS! 💣 */ FORK_FREEZE = 1 << 22, /* Don't return in child, just call freeze() instead */ + FORK_PID_ONLY = 1 << 23, /* Don't open a pidfd referencing the child process */ } ForkFlags; -int safe_fork_full( - const char *name, - const int stdio_fds[3], - int except_fds[], - size_t n_except_fds, - ForkFlags flags, - pid_t *ret_pid); - -static inline int safe_fork(const char *name, ForkFlags flags, pid_t *ret_pid) { - return safe_fork_full(name, NULL, NULL, 0, flags, ret_pid); -} - int pidref_safe_fork_full( const char *name, const int stdio_fds[3], @@ -219,6 +208,18 @@ static inline int pidref_safe_fork(const char *name, ForkFlags flags, PidRef *re return pidref_safe_fork_full(name, NULL, NULL, 0, flags, ret_pid); } +int safe_fork_full( + const char *name, + const int stdio_fds[3], + int except_fds[], + size_t n_except_fds, + ForkFlags flags, + pid_t *ret_pid); + +static inline int safe_fork(const char *name, ForkFlags flags, pid_t *ret_pid) { + return safe_fork_full(name, NULL, NULL, 0, flags, ret_pid); +} + int namespace_fork( const char *outer_name, const char *inner_name, From dc2f960b78e07b995e1e01202b0ca3f25868c724 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 19 Feb 2025 20:38:14 +0100 Subject: [PATCH 4/5] process-util: Allow setting ret_pid with FORK_DETACH in safe_fork() Let's allow getting the pid even if the caller sets FORK_DETACH. We do this via a socketpair() over which we send the inner child pid. --- src/basic/process-util.c | 60 ++++++++++++++++++++++++++++++++++-- src/test/test-process-util.c | 18 +++++++++++ 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 6a55cca625..7d86c868c3 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -36,6 +36,7 @@ #include "fs-util.h" #include "hostname-util.h" #include "io-util.h" +#include "iovec-util.h" #include "locale-util.h" #include "log.h" #include "macro.h" @@ -53,6 +54,7 @@ #include "raw-clone.h" #include "rlimit-util.h" #include "signal-util.h" +#include "socket-util.h" #include "stat-util.h" #include "stdio-util.h" #include "string-table.h" @@ -1537,10 +1539,11 @@ int pidref_safe_fork_full( sigset_t saved_ss, ss; _unused_ _cleanup_(restore_sigsetp) sigset_t *saved_ssp = NULL; bool block_signals = false, block_all = false, intermediary = false; + _cleanup_close_pair_ int pidref_transport_fds[2] = EBADF_PAIR; int prio, r; assert(!FLAGS_SET(flags, FORK_DETACH) || - (!ret_pid && (flags & (FORK_WAIT|FORK_DEATHSIG_SIGTERM|FORK_DEATHSIG_SIGINT|FORK_DEATHSIG_SIGKILL)) == 0)); + (flags & (FORK_WAIT|FORK_DEATHSIG_SIGTERM|FORK_DEATHSIG_SIGINT|FORK_DEATHSIG_SIGKILL)) == 0); /* A wrapper around fork(), that does a couple of important initializations in addition to mere * forking. If provided, ret_pid is initialized in both the parent and the child process, both times @@ -1587,14 +1590,43 @@ int pidref_safe_fork_full( if (!r) { /* Not a reaper process, hence do a double fork() so we are reparented to one */ + if (ret_pid && socketpair(AF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0, pidref_transport_fds) < 0) + return log_full_errno(prio, errno, "Failed to allocate pidref socket: %m"); + pid = fork(); if (pid < 0) return log_full_errno(prio, errno, "Failed to fork off '%s': %m", strna(name)); if (pid > 0) { log_debug("Successfully forked off intermediary '%s' as PID " PID_FMT ".", strna(name), pid); + + pidref_transport_fds[1] = safe_close(pidref_transport_fds[1]); + + if (pidref_transport_fds[0] >= 0) { + /* Wait for the intermediary child to exit so the caller can be certain the actual child + * process has been reparented by the time this function returns. */ + r = wait_for_terminate_and_check(name, pid, FLAGS_SET(flags, FORK_LOG) ? WAIT_LOG : 0); + if (r < 0) + return log_full_errno(prio, r, "Failed to wait for intermediary process: %m"); + if (r != EXIT_SUCCESS) /* exit status > 0 should be treated as failure, too */ + return -EPROTO; + + int pidfd; + ssize_t n = receive_one_fd_iov( + pidref_transport_fds[0], + &IOVEC_MAKE(&pid, sizeof(pid)), + /* iovlen= */ 1, + /* flags= */ 0, + &pidfd); + if (n < 0) + return log_full_errno(prio, n, "Failed to receive child pidref: %m"); + + *ret_pid = (PidRef) { .pid = pid, .fd = pidfd }; + } + return 1; /* return in the parent */ } + pidref_transport_fds[0] = safe_close(pidref_transport_fds[0]); intermediary = true; } } @@ -1612,8 +1644,30 @@ int pidref_safe_fork_full( if (pid > 0) { /* If we are in the intermediary process, exit now */ - if (intermediary) + if (intermediary) { + if (pidref_transport_fds[1] >= 0) { + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; + + r = pidref_set_pid(&pidref, pid); + if (r < 0) { + log_full_errno(prio, r, "Failed to open reference to PID "PID_FMT": %m", pid); + _exit(EXIT_FAILURE); + } + + r = send_one_fd_iov( + pidref_transport_fds[1], + pidref.fd, + &IOVEC_MAKE(&pidref.pid, sizeof(pidref.pid)), + /* iovlen= */ 1, + /* flags= */ 0); + if (r < 0) { + log_full_errno(prio, r, "Failed to send child pidref: %m"); + _exit(EXIT_FAILURE); + } + } + _exit(EXIT_SUCCESS); + } /* We are in the parent process */ log_debug("Successfully forked off '%s' as PID " PID_FMT ".", strna(name), pid); @@ -1654,6 +1708,8 @@ int pidref_safe_fork_full( /* We are in the child process */ + pidref_transport_fds[1] = safe_close(pidref_transport_fds[1]); + /* Restore signal mask manually */ saved_ssp = NULL; diff --git a/src/test/test-process-util.c b/src/test/test-process-util.c index 0db49fe3a8..7bd416f8d0 100644 --- a/src/test/test-process-util.c +++ b/src/test/test-process-util.c @@ -659,6 +659,24 @@ TEST(safe_fork) { ASSERT_OK(wait_for_terminate(pid, &status)); ASSERT_EQ(status.si_code, CLD_EXITED); ASSERT_EQ(status.si_status, 88); + + _cleanup_(pidref_done) PidRef child = PIDREF_NULL; + r = pidref_safe_fork("(test-child)", FORK_DETACH, &child); + if (r == 0) { + /* Don't freeze so this doesn't linger around forever in case something goes wrong. */ + usleep_safe(100 * USEC_PER_SEC); + _exit(EXIT_SUCCESS); + } + + ASSERT_OK_POSITIVE(r); + ASSERT_GT(child.pid, 0); + ASSERT_OK(pidref_get_ppid(&child, &pid)); + ASSERT_OK(pidref_kill(&child, SIGKILL)); + + if (is_reaper_process()) + ASSERT_EQ(pid, getpid_cached()); + else + ASSERT_NE(pid, getpid_cached()); } TEST(pid_to_ptr) { From 698ac172aadd15afced079bb9553e1ea24e63d06 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 19 Feb 2025 20:57:24 +0100 Subject: [PATCH 5/5] exec-invoke: Use FORK_DETACH when forking off pid namespace child This ensures the child process is immediately re-parented to the manager process which avoids a "Supervising process xxx which is not our child. We'll most likely not notice when it exits." warning which can currently happen if the parent systemd-executor parent process sends the pid namespace child process pidref to the manager process and the manager process dispatches the child process pidref before the systemd-executor parent process exits, since at that point the pid namespace child process's parent will still be the systemd-executor parent process and not the manager process. --- src/core/exec-invoke.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index 10111a8186..f0ab3637ad 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -2480,7 +2480,8 @@ static int setup_private_pids(const ExecContext *c, ExecParameters *p) { if (pipe2(errno_pipe, O_CLOEXEC) < 0) return log_exec_debug_errno(c, p, errno, "Failed to create pipe for communicating with parent process: %m"); - r = pidref_safe_fork("(sd-pidns-child)", FORK_NEW_PIDNS, &pidref); + /* Set FORK_DETACH to immediately re-parent the child process to the invoking manager process. */ + r = pidref_safe_fork("(sd-pidns-child)", FORK_NEW_PIDNS|FORK_DETACH, &pidref); if (r < 0) return log_exec_debug_errno(c, p, r, "Failed to fork child into new pid namespace: %m"); if (r > 0) {