From 37149e692a6ccf2187fa2df8bde4d59b12d979bd Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 18 Feb 2025 17:06:19 +0100 Subject: [PATCH 1/3] process-util: introduce SIGINFO_CODE_IS_DEAD helper --- src/basic/process-util.h | 4 ++++ src/core/manager.c | 2 +- src/libsystemd/sd-event/sd-event.c | 8 +++----- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/basic/process-util.h b/src/basic/process-util.h index c9b35d79f0..c3b86f064f 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -61,6 +61,10 @@ int get_process_umask(pid_t pid, mode_t *ret); int container_get_leader(const char *machine, pid_t *pid); +static inline bool SIGINFO_CODE_IS_DEAD(int code) { + return IN_SET(code, CLD_EXITED, CLD_KILLED, CLD_DUMPED); +} + int wait_for_terminate(pid_t pid, siginfo_t *status); typedef enum WaitFlags { diff --git a/src/core/manager.c b/src/core/manager.c index 72b9290dc0..c5fd03e42b 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -2990,7 +2990,7 @@ static int manager_dispatch_sigchld(sd_event_source *source, void *userdata) { if (si.si_pid <= 0) goto turn_off; - if (IN_SET(si.si_code, CLD_EXITED, CLD_KILLED, CLD_DUMPED)) { + if (SIGINFO_CODE_IS_DEAD(si.si_code)) { _cleanup_free_ char *name = NULL; (void) pid_get_comm(si.si_pid, &name); diff --git a/src/libsystemd/sd-event/sd-event.c b/src/libsystemd/sd-event/sd-event.c index 3d1b6fb228..f2abbb1b51 100644 --- a/src/libsystemd/sd-event/sd-event.c +++ b/src/libsystemd/sd-event/sd-event.c @@ -3760,7 +3760,7 @@ static int process_child(sd_event *e, int64_t threshold, int64_t *ret_min_priori return negative_errno(); if (s->child.siginfo.si_pid != 0) { - bool zombie = IN_SET(s->child.siginfo.si_code, CLD_EXITED, CLD_KILLED, CLD_DUMPED); + bool zombie = SIGINFO_CODE_IS_DEAD(s->child.siginfo.si_code); if (zombie) s->child.exited = true; @@ -3809,7 +3809,7 @@ static int process_pidfd(sd_event *e, sd_event_source *s, uint32_t revents) { if (s->child.siginfo.si_pid == 0) return 0; - if (IN_SET(s->child.siginfo.si_code, CLD_EXITED, CLD_KILLED, CLD_DUMPED)) + if (SIGINFO_CODE_IS_DEAD(s->child.siginfo.si_code)) s->child.exited = true; return source_set_pending(s, true); @@ -4222,9 +4222,7 @@ static int source_dispatch(sd_event_source *s) { break; case SOURCE_CHILD: { - bool zombie; - - zombie = IN_SET(s->child.siginfo.si_code, CLD_EXITED, CLD_KILLED, CLD_DUMPED); + bool zombie = SIGINFO_CODE_IS_DEAD(s->child.siginfo.si_code); r = s->child.callback(s, &s->child.siginfo, s->userdata); From 4f63673482000c06478d3dd655415f8348928723 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Fri, 21 Feb 2025 16:45:34 +0100 Subject: [PATCH 2/3] sd-event: assert on hashmap_remove(child.pid) Follow-up for 54988a27b9d1487e1690f94b79031ef61edd6651 and a342c71d360e083b3f6b06a64d852c4e3aceeef3 --- src/libsystemd/sd-event/sd-event.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsystemd/sd-event/sd-event.c b/src/libsystemd/sd-event/sd-event.c index f2abbb1b51..1fea7d5aa7 100644 --- a/src/libsystemd/sd-event/sd-event.c +++ b/src/libsystemd/sd-event/sd-event.c @@ -988,7 +988,7 @@ static void source_disconnect(sd_event_source *s) { s->event->n_online_child_sources--; } - (void) hashmap_remove(s->event->child_sources, PID_TO_PTR(s->child.pid)); + assert_se(hashmap_remove(s->event->child_sources, PID_TO_PTR(s->child.pid))); } if (EVENT_SOURCE_WATCH_PIDFD(s)) From c6cc7efcd35f729cf0db8f8502752c7d83a0be95 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 18 Feb 2025 16:49:05 +0100 Subject: [PATCH 3/3] sd-event: always operate on child source via pidfd Follow-up for 6e14c46bac760d01868b0bf48461f6ac44c86be3 Nowadays a pidfd is guarenteed to be around for child event sources, hence drop the effectively unused pid-based branches. Addresses https://github.com/systemd/systemd/pull/36410#discussion_r1959930716 --- man/sd_event_add_child.xml | 33 +++++------------ src/libsystemd/sd-event/sd-event.c | 59 +++++++++++++----------------- 2 files changed, 35 insertions(+), 57 deletions(-) diff --git a/man/sd_event_add_child.xml b/man/sd_event_add_child.xml index e3b9cdd746..4bf07baf59 100644 --- a/man/sd_event_add_child.xml +++ b/man/sd_event_add_child.xml @@ -201,13 +201,11 @@ sd_event_source_get_child_pidfd() retrieves the file descriptor referencing - the watched process ("pidfd") if this functionality is available. On kernels that support the concept the - event loop will make use of pidfds to watch child processes, regardless of whether the individual event sources - are allocated via sd_event_add_child() or - sd_event_add_child_pidfd(). If the latter call was used to allocate the event - source, this function returns the file descriptor used for allocation. On kernels that do not support the - pidfd concept this function will fail with EOPNOTSUPP. This call takes the event - source object as the source parameter and returns the numeric file descriptor. + the watched process ("pidfd"). The event loop internally makes use of pidfds to watch child processes, + regardless of whether the individual event sources are allocated via sd_event_add_child() + or sd_event_add_child_pidfd(). If the latter call was used to allocate the event + source, this function returns the original file descriptor used for allocation. This call takes + the event source object as the source parameter and returns the numeric file descriptor. sd_event_source_get_child_pidfd_own() may be used to query whether the pidfd @@ -228,15 +226,10 @@ exist. This behaviour might change eventually. sd_event_source_send_child_signal() may be used to send a UNIX signal to the - watched process. If the pidfd concept is supported in the kernel, this is implemented via pidfd_send_signal2 - and otherwise via rt_sigqueueinfo2 - (or via kill2 in case - info is NULL). The specified parameters match those of these - underlying system calls, except that the info is never modified (and is thus - declared constant). Like for the underlying system calls, the flags parameter + watched process via pidfd_send_signal2. + The specified parameters match those of the underlying system call, except that the info + is never modified (and is thus declared constant). Like for the underlying system call, the flags parameter currently must be zero. @@ -299,14 +292,6 @@ The passed event source is not a child process event source. - - -EOPNOTSUPP - - A pidfd was requested but the kernel does not support this concept. - - - - diff --git a/src/libsystemd/sd-event/sd-event.c b/src/libsystemd/sd-event/sd-event.c index 1fea7d5aa7..d2ef4306e2 100644 --- a/src/libsystemd/sd-event/sd-event.c +++ b/src/libsystemd/sd-event/sd-event.c @@ -43,11 +43,10 @@ #define DEFAULT_ACCURACY_USEC (250 * USEC_PER_MSEC) -static bool EVENT_SOURCE_WATCH_PIDFD(sd_event_source *s) { +static bool EVENT_SOURCE_WATCH_PIDFD(const sd_event_source *s) { /* Returns true if this is a PID event source and can be implemented by watching EPOLLIN */ return s && s->type == SOURCE_CHILD && - s->child.pidfd >= 0 && s->child.options == WEXITED; } @@ -1088,12 +1087,11 @@ static sd_event_source* source_free(sd_event_source *s) { /* Eventually the kernel will do this automatically for us, but for now let's emulate this (unreliably) in userspace. */ if (s->child.process_owned) { + assert(s->child.pid > 0); + assert(s->child.pidfd >= 0); if (!s->child.exited) { - if (s->child.pidfd >= 0) - r = RET_NERRNO(pidfd_send_signal(s->child.pidfd, SIGKILL, NULL, 0)); - else - r = RET_NERRNO(kill(s->child.pid, SIGKILL)); + r = RET_NERRNO(pidfd_send_signal(s->child.pidfd, SIGKILL, NULL, 0)); if (r < 0 && r != -ESRCH) log_debug_errno(r, "Failed to kill process " PID_FMT ", ignoring: %m", s->child.pid); @@ -1103,10 +1101,7 @@ static sd_event_source* source_free(sd_event_source *s) { siginfo_t si = {}; /* Reap the child if we can */ - if (s->child.pidfd >= 0) - (void) waitid(P_PIDFD, s->child.pidfd, &si, WEXITED); - else - (void) waitid(P_PID, s->child.pid, &si, WEXITED); + (void) waitid(P_PIDFD, s->child.pidfd, &si, WEXITED); } } @@ -2999,13 +2994,13 @@ static int event_source_online( case SOURCE_CHILD: if (EVENT_SOURCE_WATCH_PIDFD(s)) { - /* yes, we have pidfd */ + /* yes, we can rely on pidfd */ r = source_child_pidfd_register(s, enabled); if (r < 0) return r; } else { - /* no pidfd, or something other to watch for than WEXITED */ + /* something other to watch for than WEXITED */ r = event_make_signal_data(s->event, SIGCHLD, NULL); if (r < 0) { @@ -3198,9 +3193,6 @@ _public_ int sd_event_source_get_child_pidfd(sd_event_source *s) { assert_return(s->type == SOURCE_CHILD, -EDOM); assert_return(!event_origin_changed(s->event), -ECHILD); - if (s->child.pidfd < 0) - return -EOPNOTSUPP; - return s->child.pidfd; } @@ -3251,9 +3243,7 @@ _public_ int sd_event_source_get_child_pidfd_own(sd_event_source *s) { assert_return(s, -EINVAL); assert_return(s->type == SOURCE_CHILD, -EDOM); assert_return(!event_origin_changed(s->event), -ECHILD); - - if (s->child.pidfd < 0) - return -EOPNOTSUPP; + assert(s->child.pidfd >= 0); return s->child.pidfd_owned; } @@ -3262,9 +3252,7 @@ _public_ int sd_event_source_set_child_pidfd_own(sd_event_source *s, int own) { assert_return(s, -EINVAL); assert_return(s->type == SOURCE_CHILD, -EDOM); assert_return(!event_origin_changed(s->event), -ECHILD); - - if (s->child.pidfd < 0) - return -EOPNOTSUPP; + assert(s->child.pidfd >= 0); s->child.pidfd_owned = own; return 0; @@ -3723,9 +3711,9 @@ static int process_child(sd_event *e, int64_t threshold, int64_t *ret_min_priori e->need_process_child = false; - /* So, this is ugly. We iteratively invoke waitid() with P_PID + WNOHANG for each PID we wait - * for, instead of using P_ALL. This is because we only want to get child information of very - * specific child processes, and not all of them. We might not have processed the SIGCHLD event + /* So, this is ugly. We iteratively invoke waitid() + WNOHANG with each child process we shall wait for, + * instead of using P_ALL. This is because we only want to get child information of very specific + * child processes, and not all of them. We might not have processed the SIGCHLD event * of a previous invocation and we don't want to maintain a unbounded *per-child* event queue, * hence we really don't want anything flushed out of the kernel's queue that we don't care * about. Since this is O(n) this means that if you have a lot of processes you probably want @@ -3736,6 +3724,7 @@ static int process_child(sd_event *e, int64_t threshold, int64_t *ret_min_priori HASHMAP_FOREACH(s, e->child_sources) { assert(s->type == SOURCE_CHILD); + assert(s->child.pidfd >= 0); if (s->priority > threshold) continue; @@ -3755,7 +3744,7 @@ static int process_child(sd_event *e, int64_t threshold, int64_t *ret_min_priori continue; zero(s->child.siginfo); - if (waitid(P_PID, s->child.pid, &s->child.siginfo, + if (waitid(P_PIDFD, s->child.pidfd, &s->child.siginfo, WNOHANG | (s->child.options & WEXITED ? WNOWAIT : 0) | s->child.options) < 0) return negative_errno(); @@ -3764,14 +3753,12 @@ static int process_child(sd_event *e, int64_t threshold, int64_t *ret_min_priori if (zombie) s->child.exited = true; - - if (!zombie && (s->child.options & WEXITED)) { - /* If the child isn't dead then let's immediately remove the state - * change from the queue, since there's no benefit in leaving it - * queued. */ + else if (s->child.options & WEXITED) { + /* If the child isn't dead then let's immediately remove the state change + * from the queue, since there's no benefit in leaving it queued. */ assert(s->child.options & (WSTOPPED|WCONTINUED)); - (void) waitid(P_PID, s->child.pid, &s->child.siginfo, WNOHANG|(s->child.options & (WSTOPPED|WCONTINUED))); + (void) waitid(P_PIDFD, s->child.pidfd, &s->child.siginfo, WNOHANG|(s->child.options & (WSTOPPED|WCONTINUED))); } r = source_set_pending(s, true); @@ -3792,6 +3779,7 @@ static int process_pidfd(sd_event *e, sd_event_source *s, uint32_t revents) { assert(e); assert(s); assert(s->type == SOURCE_CHILD); + assert(s->child.pidfd >= 0); if (s->pending) return 0; @@ -3802,8 +3790,13 @@ static int process_pidfd(sd_event *e, sd_event_source *s, uint32_t revents) { if (!EVENT_SOURCE_WATCH_PIDFD(s)) return 0; + /* Note that pidfd would also generate EPOLLHUP when the process gets reaped. But at this point we + * only permit EPOLLIN, under the assumption that upon EPOLLHUP the child source should already + * be set to pending, and we would have returned early above. */ + assert(!s->child.exited); + zero(s->child.siginfo); - if (waitid(P_PID, s->child.pid, &s->child.siginfo, WNOHANG | WNOWAIT | s->child.options) < 0) + if (waitid(P_PIDFD, s->child.pidfd, &s->child.siginfo, WNOHANG | WNOWAIT | s->child.options) < 0) return -errno; if (s->child.siginfo.si_pid == 0) @@ -4228,7 +4221,7 @@ static int source_dispatch(sd_event_source *s) { /* Now, reap the PID for good. */ if (zombie) { - (void) waitid(P_PID, s->child.pid, &s->child.siginfo, WNOHANG|WEXITED); + (void) waitid(P_PIDFD, s->child.pidfd, &s->child.siginfo, WNOHANG|WEXITED); s->child.waited = true; }