From ca949c9dcf17ea8d6512ac4c5c1a806ded9b8dc1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 31 Aug 2010 23:24:47 +0200 Subject: [PATCH] service: rework killing logic so that we always kill the main process, even if it left our service cgroup Related to: http://bugzilla.redhat.com/show_bug.cgi?id=626477 --- src/cgroup-util.c | 28 +++++++++++++------- src/cgroup-util.h | 4 +-- src/cgroup.c | 27 ++++++++++++++------ src/cgroup.h | 4 +-- src/mount.c | 43 +++++++++++++++++++++---------- src/service.c | 65 ++++++++++++++++++++++++++++++----------------- src/socket.c | 42 ++++++++++++++++++++++-------- src/test-cgroup.c | 8 +++--- 8 files changed, 149 insertions(+), 72 deletions(-) diff --git a/src/cgroup-util.c b/src/cgroup-util.c index 6abb6b54..2167cdd6 100644 --- a/src/cgroup-util.c +++ b/src/cgroup-util.c @@ -166,12 +166,12 @@ int cg_rmdir(const char *controller, const char *path) { return r < 0 ? -errno : 0; } -int cg_kill(const char *controller, const char *path, int sig, bool ignore_self) { +int cg_kill(const char *controller, const char *path, int sig, bool ignore_self, Set *s) { bool done = false; - Set *s; int r, ret = 0; pid_t my_pid; FILE *f = NULL; + Set *allocated_set = NULL; assert(controller); assert(path); @@ -181,8 +181,9 @@ int cg_kill(const char *controller, const char *path, int sig, bool ignore_self) * is repeated until no further processes are added to the * tasks list, to properly handle forking processes */ - if (!(s = set_new(trivial_hash_func, trivial_compare_func))) - return -ENOMEM; + if (!s) + if (!(s = allocated_set = set_new(trivial_hash_func, trivial_compare_func))) + return -ENOMEM; my_pid = getpid(); @@ -240,7 +241,8 @@ int cg_kill(const char *controller, const char *path, int sig, bool ignore_self) } while (!done); finish: - set_free(s); + if (allocated_set) + set_free(allocated_set); if (f) fclose(f); @@ -248,16 +250,21 @@ finish: return ret; } -int cg_kill_recursive(const char *controller, const char *path, int sig, bool ignore_self, bool rem) { +int cg_kill_recursive(const char *controller, const char *path, int sig, bool ignore_self, bool rem, Set *s) { int r, ret = 0; DIR *d = NULL; char *fn; + Set *allocated_set = NULL; assert(path); assert(controller); assert(sig >= 0); - ret = cg_kill(controller, path, sig, ignore_self); + if (!s) + if (!(s = allocated_set = set_new(trivial_hash_func, trivial_compare_func))) + return -ENOMEM; + + ret = cg_kill(controller, path, sig, ignore_self, s); if ((r = cg_enumerate_subgroups(controller, path, &d)) < 0) { if (ret >= 0 && r != -ENOENT) @@ -279,7 +286,7 @@ int cg_kill_recursive(const char *controller, const char *path, int sig, bool ig goto finish; } - r = cg_kill_recursive(controller, p, sig, ignore_self, rem); + r = cg_kill_recursive(controller, p, sig, ignore_self, rem, s); free(p); if (r != 0 && ret >= 0) @@ -299,6 +306,9 @@ finish: if (d) closedir(d); + if (allocated_set) + set_free(allocated_set); + return ret; } @@ -323,7 +333,7 @@ int cg_kill_recursive_and_wait(const char *controller, const char *path, bool re else sig = 0; - if ((r = cg_kill_recursive(controller, path, sig, true, rem)) <= 0) + if ((r = cg_kill_recursive(controller, path, sig, true, rem, NULL)) <= 0) return r; usleep(50 * USEC_PER_MSEC); diff --git a/src/cgroup-util.h b/src/cgroup-util.h index d68ccc5f..2cb6ede4 100644 --- a/src/cgroup-util.h +++ b/src/cgroup-util.h @@ -37,8 +37,8 @@ int cg_read_pid(FILE *f, pid_t *_pid); int cg_enumerate_subgroups(const char *controller, const char *path, DIR **_d); int cg_read_subgroup(DIR *d, char **fn); -int cg_kill(const char *controller, const char *path, int sig, bool ignore_self); -int cg_kill_recursive(const char *controller, const char *path, int sig, bool ignore_self, bool remove); +int cg_kill(const char *controller, const char *path, int sig, bool ignore_self, Set *s); +int cg_kill_recursive(const char *controller, const char *path, int sig, bool ignore_self, bool remove, Set *s); int cg_kill_recursive_and_wait(const char *controller, const char *path, bool remove); int cg_migrate(const char *controller, const char *from, const char *to, bool ignore_self); diff --git a/src/cgroup.c b/src/cgroup.c index c1ea95ab..d64ab63b 100644 --- a/src/cgroup.c +++ b/src/cgroup.c @@ -138,7 +138,7 @@ int cgroup_bonding_install_list(CGroupBonding *first, pid_t pid) { return 0; } -int cgroup_bonding_kill(CGroupBonding *b, int sig) { +int cgroup_bonding_kill(CGroupBonding *b, int sig, Set *s) { int r; assert(b); @@ -149,25 +149,36 @@ int cgroup_bonding_kill(CGroupBonding *b, int sig) { assert(b->realized); - return cg_kill_recursive(b->controller, b->path, sig, true, false); + return cg_kill_recursive(b->controller, b->path, sig, true, false, s); } -int cgroup_bonding_kill_list(CGroupBonding *first, int sig) { +int cgroup_bonding_kill_list(CGroupBonding *first, int sig, Set *s) { CGroupBonding *b; - int r = -EAGAIN; + Set *allocated_set = NULL; + int ret = -EAGAIN, r; + + if (!s) + if (!(s = allocated_set = set_new(trivial_hash_func, trivial_compare_func))) + return -ENOMEM; LIST_FOREACH(by_unit, b, first) { - if ((r = cgroup_bonding_kill(b, sig)) < 0) { + if ((r = cgroup_bonding_kill(b, sig, s)) < 0) { if (r == -EAGAIN || r == -ESRCH) continue; - return r; + ret = r; + goto finish; } - return 0; + if (ret < 0 || r > 0) + ret = r; } - return r; +finish: + if (allocated_set) + set_free(allocated_set); + + return ret; } /* Returns 1 if the group is empty, 0 if it is not, -EAGAIN if we diff --git a/src/cgroup.h b/src/cgroup.h index 90c4572d..e38d7e79 100644 --- a/src/cgroup.h +++ b/src/cgroup.h @@ -58,8 +58,8 @@ void cgroup_bonding_free_list(CGroupBonding *first); int cgroup_bonding_install(CGroupBonding *b, pid_t pid); int cgroup_bonding_install_list(CGroupBonding *first, pid_t pid); -int cgroup_bonding_kill(CGroupBonding *b, int sig); -int cgroup_bonding_kill_list(CGroupBonding *first, int sig); +int cgroup_bonding_kill(CGroupBonding *b, int sig, Set *s); +int cgroup_bonding_kill_list(CGroupBonding *first, int sig, Set *s); void cgroup_bonding_trim(CGroupBonding *first, bool delete_root); void cgroup_bonding_trim_list(CGroupBonding *first, bool delete_root); diff --git a/src/mount.c b/src/mount.c index 2b6274eb..d651e871 100644 --- a/src/mount.c +++ b/src/mount.c @@ -622,7 +622,8 @@ static void mount_enter_mounted(Mount *m, bool success) { static void mount_enter_signal(Mount *m, MountState state, bool success) { int r; - bool sent = false; + Set *pid_set = NULL; + bool wait_for_exit = false; assert(m); @@ -634,26 +635,39 @@ static void mount_enter_signal(Mount *m, MountState state, bool success) { state == MOUNT_UNMOUNTING_SIGTERM || state == MOUNT_REMOUNTING_SIGTERM) ? m->exec_context.kill_signal : SIGKILL; - if (m->exec_context.kill_mode == KILL_CONTROL_GROUP) { + if (m->control_pid > 0) { + if (kill(m->exec_context.kill_mode == KILL_PROCESS_GROUP ? + -m->control_pid : + m->control_pid, sig) < 0 && errno != ESRCH) - if ((r = cgroup_bonding_kill_list(m->meta.cgroup_bondings, sig)) < 0) { - if (r != -EAGAIN && r != -ESRCH) - goto fail; - } else - sent = true; + log_warning("Failed to kill control process %li: %m", (long) m->control_pid); + else + wait_for_exit = true; } - if (!sent && m->control_pid > 0) - if (kill(m->exec_context.kill_mode == KILL_PROCESS ? - m->control_pid : - -m->control_pid, sig) < 0 && errno != ESRCH) { + if (m->exec_context.kill_mode == KILL_CONTROL_GROUP) { - r = -errno; + if (!(pid_set = set_new(trivial_hash_func, trivial_compare_func))) { + r = -ENOMEM; goto fail; } + + /* Exclude the control pid from being killed via the cgroup */ + if (m->control_pid > 0) + if ((r = set_put(pid_set, LONG_TO_PTR(m->control_pid))) < 0) + goto fail; + + if ((r = cgroup_bonding_kill_list(m->meta.cgroup_bondings, sig, pid_set)) < 0) { + if (r != -EAGAIN && r != -ESRCH && r != -ENOENT) + log_warning("Failed to kill control group: %s", strerror(-r)); + } else if (r > 0) + wait_for_exit = true; + + set_free(pid_set); + } } - if (sent) { + if (wait_for_exit) { if ((r = unit_watch_timer(UNIT(m), m->timeout_usec, &m->timer_watch)) < 0) goto fail; @@ -672,6 +686,9 @@ fail: mount_enter_mounted(m, false); else mount_enter_dead(m, false); + + if (pid_set) + set_free(pid_set); } static void mount_enter_unmounting(Mount *m, bool success) { diff --git a/src/service.c b/src/service.c index 88992eb4..071f015b 100644 --- a/src/service.c +++ b/src/service.c @@ -1570,7 +1570,8 @@ fail: static void service_enter_signal(Service *s, ServiceState state, bool success) { int r; - bool sent = false; + Set *pid_set = NULL; + bool wait_for_exit = false; assert(s); @@ -1580,38 +1581,53 @@ static void service_enter_signal(Service *s, ServiceState state, bool success) { if (s->exec_context.kill_mode != KILL_NONE) { int sig = (state == SERVICE_STOP_SIGTERM || state == SERVICE_FINAL_SIGTERM) ? s->exec_context.kill_signal : SIGKILL; - if (s->exec_context.kill_mode == KILL_CONTROL_GROUP) { + if (s->main_pid > 0) { + if (kill(s->exec_context.kill_mode == KILL_PROCESS_GROUP ? + -s->main_pid : + s->main_pid, sig) < 0 && errno != ESRCH) - if ((r = cgroup_bonding_kill_list(s->meta.cgroup_bondings, sig)) < 0) { - if (r != -EAGAIN && r != -ESRCH) - goto fail; - } else - sent = true; + log_warning("Failed to kill main process %li: %m", (long) s->main_pid); + else + wait_for_exit = true; } - if (!sent) { - r = 0; + if (s->control_pid > 0) { + if (kill(s->exec_context.kill_mode == KILL_PROCESS_GROUP ? + -s->control_pid : + s->control_pid, sig) < 0 && errno != ESRCH) - if (s->main_pid > 0) { - if (kill(s->exec_context.kill_mode == KILL_PROCESS ? s->main_pid : -s->main_pid, sig) < 0 && errno != ESRCH) - r = -errno; - else - sent = true; - } + log_warning("Failed to kill control process %li: %m", (long) s->control_pid); + else + wait_for_exit = true; + } - if (s->control_pid > 0) { - if (kill(s->exec_context.kill_mode == KILL_PROCESS ? s->control_pid : -s->control_pid, sig) < 0 && errno != ESRCH) - r = -errno; - else - sent = true; - } + if (s->exec_context.kill_mode == KILL_CONTROL_GROUP) { - if (r < 0) + if (!(pid_set = set_new(trivial_hash_func, trivial_compare_func))) { + r = -ENOMEM; goto fail; + } + + /* Exclude the main/control pids from being killed via the cgroup */ + if (s->main_pid > 0) + if ((r = set_put(pid_set, LONG_TO_PTR(s->main_pid))) < 0) + goto fail; + + if (s->control_pid > 0) + if ((r = set_put(pid_set, LONG_TO_PTR(s->control_pid))) < 0) + goto fail; + + if ((r = cgroup_bonding_kill_list(s->meta.cgroup_bondings, sig, pid_set)) < 0) { + if (r != -EAGAIN && r != -ESRCH && r != -ENOENT) + log_warning("Failed to kill control group: %s", strerror(-r)); + } else + wait_for_exit = true; + + set_free(pid_set); } } - if (sent && (s->main_pid > 0 || s->control_pid > 0)) { + if (wait_for_exit) { if (s->timeout_usec > 0) if ((r = unit_watch_timer(UNIT(s), s->timeout_usec, &s->timer_watch)) < 0) goto fail; @@ -1631,6 +1647,9 @@ fail: service_enter_stop_post(s, false); else service_enter_dead(s, false, true); + + if (pid_set) + set_free(pid_set); } static void service_enter_stop(Service *s, bool success) { diff --git a/src/socket.c b/src/socket.c index c4a95579..fd975fd9 100644 --- a/src/socket.c +++ b/src/socket.c @@ -997,7 +997,8 @@ fail: static void socket_enter_signal(Socket *s, SocketState state, bool success) { int r; - bool sent = false; + Set *pid_set = NULL; + bool wait_for_exit = false; assert(s); @@ -1007,23 +1008,39 @@ static void socket_enter_signal(Socket *s, SocketState state, bool success) { if (s->exec_context.kill_mode != KILL_NONE) { int sig = (state == SOCKET_STOP_PRE_SIGTERM || state == SOCKET_FINAL_SIGTERM) ? s->exec_context.kill_signal : SIGKILL; - if (s->exec_context.kill_mode == KILL_CONTROL_GROUP) { + if (s->control_pid > 0) { + if (kill(s->exec_context.kill_mode == KILL_PROCESS_GROUP ? + -s->control_pid : + s->control_pid, sig) < 0 && errno != ESRCH) - if ((r = cgroup_bonding_kill_list(s->meta.cgroup_bondings, sig)) < 0) { - if (r != -EAGAIN && r != -ESRCH) - goto fail; - } else - sent = true; + log_warning("Failed to kill control process %li: %m", (long) s->control_pid); + else + wait_for_exit = true; } - if (!sent && s->control_pid > 0) - if (kill(s->exec_context.kill_mode == KILL_PROCESS ? s->control_pid : -s->control_pid, sig) < 0 && errno != ESRCH) { - r = -errno; + if (s->exec_context.kill_mode == KILL_CONTROL_GROUP) { + + if (!(pid_set = set_new(trivial_hash_func, trivial_compare_func))) { + r = -ENOMEM; goto fail; } + + /* Exclude the control pid from being killed via the cgroup */ + if (s->control_pid > 0) + if ((r = set_put(pid_set, LONG_TO_PTR(s->control_pid))) < 0) + goto fail; + + if ((r = cgroup_bonding_kill_list(s->meta.cgroup_bondings, sig, pid_set)) < 0) { + if (r != -EAGAIN && r != -ESRCH && r != -ENOENT) + log_warning("Failed to kill control group: %s", strerror(-r)); + } else if (r > 0) + wait_for_exit = true; + + set_free(pid_set); + } } - if (sent && s->control_pid > 0) { + if (wait_for_exit) { if ((r = unit_watch_timer(UNIT(s), s->timeout_usec, &s->timer_watch)) < 0) goto fail; @@ -1042,6 +1059,9 @@ fail: socket_enter_stop_post(s, false); else socket_enter_dead(s, false); + + if (pid_set) + set_free(pid_set); } static void socket_enter_stop_pre(Socket *s, bool success) { diff --git a/src/test-cgroup.c b/src/test-cgroup.c index 63329a39..486656b4 100644 --- a/src/test-cgroup.c +++ b/src/test-cgroup.c @@ -61,16 +61,16 @@ int main(int argc, char*argv[]) { assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", false) > 0); assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", false) == 0); - assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", 0, false, false) == 0); - assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", 0, false, false) > 0); + assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", 0, false, false, NULL) == 0); + assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", 0, false, false, NULL) > 0); assert_se(cg_migrate_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", "/test-a", false, false) > 0); assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", false) == 0); assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", false) > 0); - assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", 0, false, false) > 0); - assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", 0, false, false) == 0); + assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", 0, false, false, NULL) > 0); + assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", 0, false, false, NULL) == 0); cg_trim(SYSTEMD_CGROUP_CONTROLLER, "/", false); -- 2.39.5