diff --git a/src/basic/process-util.c b/src/basic/process-util.c index c41a2aa5c9..98ab8b005c 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -934,6 +934,20 @@ int getenv_for_pid(pid_t pid, const char *field, char **ret) { return 0; } +int pid_is_my_child(pid_t pid) { + pid_t ppid; + int r; + + if (pid <= 1) + return false; + + r = get_process_ppid(pid, &ppid); + if (r < 0) + return r; + + return ppid == getpid_cached(); +} + bool pid_is_unwaited(pid_t pid) { /* Checks whether a PID is still valid at all, including a zombie */ diff --git a/src/basic/process-util.h b/src/basic/process-util.h index 9950723996..1571f1a652 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -69,6 +69,7 @@ int getenv_for_pid(pid_t pid, const char *field, char **_value); bool pid_is_alive(pid_t pid); bool pid_is_unwaited(pid_t pid); +int pid_is_my_child(pid_t pid); int pid_from_same_root_fs(pid_t pid); bool is_main_thread(void); diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 059bf29eb9..ad67ba0438 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -2269,7 +2269,7 @@ void unit_prune_cgroup(Unit *u) { int unit_search_main_pid(Unit *u, pid_t *ret) { _cleanup_fclose_ FILE *f = NULL; - pid_t pid = 0, npid, mypid; + pid_t pid = 0, npid; int r; assert(u); @@ -2282,15 +2282,12 @@ int unit_search_main_pid(Unit *u, pid_t *ret) { if (r < 0) return r; - mypid = getpid_cached(); while (cg_read_pid(f, &npid) > 0) { - pid_t ppid; if (npid == pid) continue; - /* Ignore processes that aren't our kids */ - if (get_process_ppid(npid, &ppid) >= 0 && ppid != mypid) + if (pid_is_my_child(npid) == 0) continue; if (pid != 0) @@ -2322,7 +2319,7 @@ static int unit_watch_pids_in_path(Unit *u, const char *path) { pid_t pid; while ((r = cg_read_pid(f, &pid)) > 0) { - r = unit_watch_pid(u, pid); + r = unit_watch_pid(u, pid, false); if (r < 0 && ret >= 0) ret = r; } diff --git a/src/core/dbus-scope.c b/src/core/dbus-scope.c index bb807df2e9..8eb915e508 100644 --- a/src/core/dbus-scope.c +++ b/src/core/dbus-scope.c @@ -106,7 +106,7 @@ static int bus_scope_set_transient_property( return r; if (!UNIT_WRITE_FLAGS_NOOP(flags)) { - r = unit_watch_pid(UNIT(s), pid); + r = unit_watch_pid(UNIT(s), pid, false); if (r < 0 && r != -EEXIST) return r; } diff --git a/src/core/manager.c b/src/core/manager.c index 22d1fda2dc..33785d905f 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -2129,6 +2129,16 @@ void manager_clear_jobs(Manager *m) { job_finish_and_invalidate(j, JOB_CANCELED, false, false); } +void manager_unwatch_pid(Manager *m, pid_t pid) { + assert(m); + + /* First let's drop the unit keyed as "pid". */ + (void) hashmap_remove(m->watch_pids, PID_TO_PTR(pid)); + + /* Then, let's also drop the array keyed by -pid. */ + free(hashmap_remove(m->watch_pids, PID_TO_PTR(-pid))); +} + static int manager_dispatch_run_queue(sd_event_source *source, void *userdata) { Manager *m = userdata; Job *j; diff --git a/src/core/manager.h b/src/core/manager.h index bce8020cfd..c43c587a79 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -437,6 +437,8 @@ int manager_get_dump_string(Manager *m, char **ret); void manager_clear_jobs(Manager *m); +void manager_unwatch_pid(Manager *m, pid_t pid); + unsigned manager_dispatch_load_queue(Manager *m); int manager_default_environment(Manager *m); diff --git a/src/core/mount.c b/src/core/mount.c index a991bf8794..4fb4b0f81e 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -704,7 +704,7 @@ static int mount_coldplug(Unit *u) { pid_is_unwaited(m->control_pid) && MOUNT_STATE_WITH_PROCESS(new_state)) { - r = unit_watch_pid(UNIT(m), m->control_pid); + r = unit_watch_pid(UNIT(m), m->control_pid, false); if (r < 0) return r; @@ -810,9 +810,8 @@ static int mount_spawn(Mount *m, ExecCommand *c, pid_t *_pid) { if (r < 0) return r; - r = unit_watch_pid(UNIT(m), pid); + r = unit_watch_pid(UNIT(m), pid, true); if (r < 0) - /* FIXME: we need to do something here */ return r; *_pid = pid; diff --git a/src/core/scope.c b/src/core/scope.c index e478661f94..7f83052258 100644 --- a/src/core/scope.c +++ b/src/core/scope.c @@ -333,11 +333,11 @@ static int scope_start(Unit *u) { (void) unit_reset_cpu_accounting(u); (void) unit_reset_ip_accounting(u); - unit_export_state_files(UNIT(s)); + unit_export_state_files(u); - r = unit_attach_pids_to_cgroup(u, UNIT(s)->pids, NULL); + r = unit_attach_pids_to_cgroup(u, u->pids, NULL); if (r < 0) { - log_unit_warning_errno(UNIT(s), r, "Failed to add PIDs to scope's control group: %m"); + log_unit_warning_errno(u, r, "Failed to add PIDs to scope's control group: %m"); scope_enter_dead(s, SCOPE_FAILURE_RESOURCES); return r; } @@ -347,7 +347,7 @@ static int scope_start(Unit *u) { scope_set_state(s, SCOPE_RUNNING); /* Start watching the PIDs currently in the scope */ - (void) unit_enqueue_rewatch_pids(UNIT(s)); + (void) unit_enqueue_rewatch_pids(u); return 1; } diff --git a/src/core/service.c b/src/core/service.c index bbe5532209..25a1297569 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -143,8 +143,6 @@ static void service_unwatch_pid_file(Service *s) { } static int service_set_main_pid(Service *s, pid_t pid) { - pid_t ppid; - assert(s); if (pid <= 1) @@ -163,12 +161,10 @@ static int service_set_main_pid(Service *s, pid_t pid) { s->main_pid = pid; s->main_pid_known = true; + s->main_pid_alien = pid_is_my_child(pid) == 0; - if (get_process_ppid(pid, &ppid) >= 0 && ppid != getpid_cached()) { + if (s->main_pid_alien) log_unit_warning(UNIT(s), "Supervising process "PID_FMT" which is not our child. We'll most likely not notice when it exits.", pid); - s->main_pid_alien = true; - } else - s->main_pid_alien = false; return 0; } @@ -994,7 +990,7 @@ static int service_load_pid_file(Service *s, bool may_warn) { if (r < 0) return r; - r = unit_watch_pid(UNIT(s), pid); + r = unit_watch_pid(UNIT(s), pid, false); if (r < 0) /* FIXME: we need to do something here */ return log_unit_warning_errno(UNIT(s), r, "Failed to watch PID "PID_FMT" for service: %m", pid); @@ -1024,7 +1020,7 @@ static void service_search_main_pid(Service *s) { if (service_set_main_pid(s, pid) < 0) return; - r = unit_watch_pid(UNIT(s), pid); + r = unit_watch_pid(UNIT(s), pid, false); if (r < 0) /* FIXME: we need to do something here */ log_unit_warning_errno(UNIT(s), r, "Failed to watch PID "PID_FMT" from: %m", pid); @@ -1158,7 +1154,7 @@ static int service_coldplug(Unit *u) { SERVICE_RUNNING, SERVICE_RELOAD, SERVICE_STOP, SERVICE_STOP_WATCHDOG, SERVICE_STOP_SIGTERM, SERVICE_STOP_SIGKILL, SERVICE_STOP_POST, SERVICE_FINAL_SIGTERM, SERVICE_FINAL_SIGKILL))) { - r = unit_watch_pid(UNIT(s), s->main_pid); + r = unit_watch_pid(UNIT(s), s->main_pid, false); if (r < 0) return r; } @@ -1170,7 +1166,7 @@ static int service_coldplug(Unit *u) { SERVICE_RELOAD, SERVICE_STOP, SERVICE_STOP_WATCHDOG, SERVICE_STOP_SIGTERM, SERVICE_STOP_SIGKILL, SERVICE_STOP_POST, SERVICE_FINAL_SIGTERM, SERVICE_FINAL_SIGKILL)) { - r = unit_watch_pid(UNIT(s), s->control_pid); + r = unit_watch_pid(UNIT(s), s->control_pid, false); if (r < 0) return r; } @@ -1570,8 +1566,8 @@ static int service_spawn( s->exec_fd_event_source = TAKE_PTR(exec_fd_source); s->exec_fd_hot = false; - r = unit_watch_pid(UNIT(s), pid); - if (r < 0) /* FIXME: we need to do something here */ + r = unit_watch_pid(UNIT(s), pid, true); + if (r < 0) return r; *_pid = pid; @@ -3475,8 +3471,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { if (main_pid_good(s) <= 0) service_enter_stop_post(s, f); - /* If there is still a service - * process around, wait until + /* If there is still a service process around, wait until * that one quit, too */ break; @@ -3498,10 +3493,14 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { if (notify_dbus) unit_add_to_dbus_queue(u); - /* If we get a SIGCHLD event for one of the processes we were interested in, then we look for others to watch, - * under the assumption that we'll sooner or later get a SIGCHLD for them, as the original process we watched - * was probably the parent of them, and they are hence now our children. */ - (void) unit_enqueue_rewatch_pids(u); + /* We watch the main/control process otherwise we can't retrieve the unit they + * belong to with cgroupv1. But if they are not our direct child, we won't get a + * SIGCHLD for them. Therefore we need to look for others to watch so we can + * detect when the cgroup becomes empty. Note that the control process is always + * our child so it's pointless to watch all other processes. */ + if (!control_pid_good(s)) + if (!s->main_pid_known || s->main_pid_alien) + (void) unit_enqueue_rewatch_pids(u); } static int service_dispatch_timer(sd_event_source *source, usec_t usec, void *userdata) { @@ -3709,7 +3708,7 @@ static void service_notify_message( if (r > 0) { service_set_main_pid(s, new_main_pid); - r = unit_watch_pid(UNIT(s), new_main_pid); + r = unit_watch_pid(UNIT(s), new_main_pid, false); if (r < 0) log_unit_warning_errno(UNIT(s), r, "Failed to watch new main PID "PID_FMT" for service: %m", new_main_pid); @@ -3927,7 +3926,7 @@ static void service_bus_name_owner_change( log_unit_debug(u, "D-Bus name %s is now owned by process " PID_FMT, name, pid); service_set_main_pid(s, pid); - unit_watch_pid(UNIT(s), pid); + unit_watch_pid(UNIT(s), pid, false); } } } diff --git a/src/core/socket.c b/src/core/socket.c index fd550290be..1396842b26 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -1852,7 +1852,7 @@ static int socket_coldplug(Unit *u) { SOCKET_FINAL_SIGTERM, SOCKET_FINAL_SIGKILL)) { - r = unit_watch_pid(UNIT(s), s->control_pid); + r = unit_watch_pid(UNIT(s), s->control_pid, false); if (r < 0) return r; @@ -1938,9 +1938,8 @@ static int socket_spawn(Socket *s, ExecCommand *c, pid_t *_pid) { if (r < 0) return r; - r = unit_watch_pid(UNIT(s), pid); + r = unit_watch_pid(UNIT(s), pid, true); if (r < 0) - /* FIXME: we need to do something here */ return r; *_pid = pid; @@ -2009,7 +2008,7 @@ static int socket_chown(Socket *s, pid_t *_pid) { _exit(EXIT_SUCCESS); } - r = unit_watch_pid(UNIT(s), pid); + r = unit_watch_pid(UNIT(s), pid, true); if (r < 0) goto fail; diff --git a/src/core/swap.c b/src/core/swap.c index b836fdd304..28acef2373 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -550,7 +550,7 @@ static int swap_coldplug(Unit *u) { pid_is_unwaited(s->control_pid) && SWAP_STATE_WITH_PROCESS(new_state)) { - r = unit_watch_pid(UNIT(s), s->control_pid); + r = unit_watch_pid(UNIT(s), s->control_pid, false); if (r < 0) return r; @@ -657,9 +657,8 @@ static int swap_spawn(Swap *s, ExecCommand *c, pid_t *_pid) { if (r < 0) goto fail; - r = unit_watch_pid(UNIT(s), pid); + r = unit_watch_pid(UNIT(s), pid, true); if (r < 0) - /* FIXME: we need to do something here */ goto fail; *_pid = pid; diff --git a/src/core/unit.c b/src/core/unit.c index 445cf6695c..5620fd9017 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2525,7 +2525,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag unit_add_to_gc_queue(u); } -int unit_watch_pid(Unit *u, pid_t pid) { +int unit_watch_pid(Unit *u, pid_t pid, bool exclusive) { int r; assert(u); @@ -2533,6 +2533,12 @@ int unit_watch_pid(Unit *u, pid_t pid) { /* Watch a specific PID */ + /* Caller might be sure that this PID belongs to this unit only. Let's take this + * opportunity to remove any stalled references to this PID as they can be created + * easily (when watching a process which is not our direct child). */ + if (exclusive) + manager_unwatch_pid(u->manager, pid); + r = set_ensure_allocated(&u->pids, NULL); if (r < 0) return r; diff --git a/src/core/unit.h b/src/core/unit.h index 35b936771c..28878b5f4f 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -678,7 +678,7 @@ typedef enum UnitNotifyFlags { void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlags flags); -int unit_watch_pid(Unit *u, pid_t pid); +int unit_watch_pid(Unit *u, pid_t pid, bool exclusive); void unit_unwatch_pid(Unit *u, pid_t pid); void unit_unwatch_all_pids(Unit *u); diff --git a/src/test/test-watch-pid.c b/src/test/test-watch-pid.c index 2c6ca0a1a2..04d7e1fbd8 100644 --- a/src/test/test-watch-pid.c +++ b/src/test/test-watch-pid.c @@ -42,25 +42,25 @@ int main(int argc, char *argv[]) { assert_se(hashmap_isempty(m->watch_pids)); assert_se(manager_get_unit_by_pid(m, 4711) == NULL); - assert_se(unit_watch_pid(a, 4711) >= 0); + assert_se(unit_watch_pid(a, 4711, false) >= 0); assert_se(manager_get_unit_by_pid(m, 4711) == a); - assert_se(unit_watch_pid(a, 4711) >= 0); + assert_se(unit_watch_pid(a, 4711, false) >= 0); assert_se(manager_get_unit_by_pid(m, 4711) == a); - assert_se(unit_watch_pid(b, 4711) >= 0); + assert_se(unit_watch_pid(b, 4711, false) >= 0); u = manager_get_unit_by_pid(m, 4711); assert_se(u == a || u == b); - assert_se(unit_watch_pid(b, 4711) >= 0); + assert_se(unit_watch_pid(b, 4711, false) >= 0); u = manager_get_unit_by_pid(m, 4711); assert_se(u == a || u == b); - assert_se(unit_watch_pid(c, 4711) >= 0); + assert_se(unit_watch_pid(c, 4711, false) >= 0); u = manager_get_unit_by_pid(m, 4711); assert_se(u == a || u == b || u == c); - assert_se(unit_watch_pid(c, 4711) >= 0); + assert_se(unit_watch_pid(c, 4711, false) >= 0); u = manager_get_unit_by_pid(m, 4711); assert_se(u == a || u == b || u == c);