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

8 files changed, 149 insertions, 72 deletions

diff --git a/src/cgroup-util.c b/src/cgroup-util.c
index 6abb6b549a..2167cdd6d0 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 d68ccc5f40..2cb6ede4a4 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 c1ea95ab56..d64ab63bea 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 90c4572d65..e38d7e79e6 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 2b6274ebcb..d651e8714b 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 88992eb42a..071f015b13 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 c4a95579b5..fd975fd99b 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 63329a397c..486656b41d 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);