service: place control command in subcgroup control/

Previously, we were brutally and onconditionally killing all processes
in a service's cgroup before starting the service anew, in order to
ensure that StartPre lines cannot be misused to spawn long-running
processes.

On logind-less systems this has the effect that restarting sshd
necessarily calls all active ssh sessions, which is usually not
desirable.

With this patch control processes for a service are placed in a
sub-cgroup called "control/". When starting a service anew we simply
kill this cgroup, but not the main cgroup, in order to avoid killing any
long-running non-control processes from previous runs.

https://bugzilla.redhat.com/show_bug.cgi?id=805942

diff --git a/TODO b/TODO
index fa0905657ced75b684e5c5ab2f115ceb1e6f23b6..12ce158ece1bfd8ee06872a00aaacbaf52895e43 100644 (file)
--- a/TODO
+++ b/TODO
@@ -19,8 +19,6 @@ Features:
 
 * cg_create_and_attach() should fail for non-available controllers
 
-* place start-pre/start-post/... scripts in sub cgrouprs
-
 * make gtk-doc optional (like kmod?)
 
 * udev: find a way to tell udev to not cancel firmware requests in initramfs

diff --git a/src/core/cgroup.c b/src/core/cgroup.c
index ef9b02f4639000039d0e19346049d178ce7b03c6..7a5f673a7ad0c7e4731dda7bd3f9b08baf8a2254 100644 (file)
--- a/src/core/cgroup.c
+++ b/src/core/cgroup.c
@@ -108,26 +108,43 @@ void cgroup_bonding_trim_list(CGroupBonding *first, bool delete_root) {
                 cgroup_bonding_trim(b, delete_root);
 }
 
-int cgroup_bonding_install(CGroupBonding *b, pid_t pid) {
+
+int cgroup_bonding_install(CGroupBonding *b, pid_t pid, const char *cgroup_suffix) {
+        char *p = NULL;
+        const char *path;
         int r;
 
         assert(b);
         assert(pid >= 0);
 
-        if ((r = cg_create_and_attach(b->controller, b->path, pid)) < 0)
+        if (cgroup_suffix) {
+                p = join(b->path, "/", cgroup_suffix, NULL);
+                if (!p)
+                        return -ENOMEM;
+
+                path = p;
+        } else
+                path = b->path;
+
+        r = cg_create_and_attach(b->controller, path, pid);
+        free(p);
+
+        if (r < 0)
                 return r;
 
         b->realized = true;
         return 0;
 }
 
-int cgroup_bonding_install_list(CGroupBonding *first, pid_t pid) {
+int cgroup_bonding_install_list(CGroupBonding *first, pid_t pid, const char *cgroup_suffix) {
         CGroupBonding *b;
         int r;
 
-        LIST_FOREACH(by_unit, b, first)
-                if ((r = cgroup_bonding_install(b, pid)) < 0 && b->essential)
+        LIST_FOREACH(by_unit, b, first) {
+                r = cgroup_bonding_install(b, pid, cgroup_suffix);
+                if (r < 0 && b->essential)
                         return r;
+        }
 
         return 0;
 }
@@ -176,7 +193,11 @@ int cgroup_bonding_set_task_access_list(CGroupBonding *first, mode_t mode, uid_t
         return 0;
 }
 
-int cgroup_bonding_kill(CGroupBonding *b, int sig, bool sigcont, Set *s) {
+int cgroup_bonding_kill(CGroupBonding *b, int sig, bool sigcont, Set *s, const char *cgroup_suffix) {
+        char *p = NULL;
+        const char *path;
+        int r;
+
         assert(b);
         assert(sig >= 0);
 
@@ -184,10 +205,22 @@ int cgroup_bonding_kill(CGroupBonding *b, int sig, bool sigcont, Set *s) {
         if (!b->ours)
                 return 0;
 
-        return cg_kill_recursive(b->controller, b->path, sig, sigcont, true, false, s);
+        if (cgroup_suffix) {
+                p = join(b->path, "/", cgroup_suffix, NULL);
+                if (!p)
+                        return -ENOMEM;
+
+                path = p;
+        } else
+                path = b->path;
+
+        r = cg_kill_recursive(b->controller, path, sig, sigcont, true, false, s);
+        free(p);
+
+        return r;
 }
 
-int cgroup_bonding_kill_list(CGroupBonding *first, int sig, bool sigcont, Set *s) {
+int cgroup_bonding_kill_list(CGroupBonding *first, int sig, bool sigcont, Set *s, const char *cgroup_suffix) {
         CGroupBonding *b;
         Set *allocated_set = NULL;
         int ret = -EAGAIN, r;
@@ -200,7 +233,8 @@ int cgroup_bonding_kill_list(CGroupBonding *first, int sig, bool sigcont, Set *s
                         return -ENOMEM;
 
         LIST_FOREACH(by_unit, b, first) {
-                if ((r = cgroup_bonding_kill(b, sig, sigcont, s)) < 0) {
+                r = cgroup_bonding_kill(b, sig, sigcont, s, cgroup_suffix);
+                if (r < 0) {
                         if (r == -EAGAIN || r == -ESRCH)
                                 continue;
 

diff --git a/src/core/cgroup.h b/src/core/cgroup.h
index de248fbca39a64704bf091637a20e819904a524a..95f09e00195f69dd700544f36c928cb026d4624a 100644 (file)
--- a/src/core/cgroup.h
+++ b/src/core/cgroup.h
@@ -56,8 +56,8 @@ int cgroup_bonding_realize_list(CGroupBonding *first);
 void cgroup_bonding_free(CGroupBonding *b, bool trim);
 void cgroup_bonding_free_list(CGroupBonding *first, bool trim);
 
-int cgroup_bonding_install(CGroupBonding *b, pid_t pid);
-int cgroup_bonding_install_list(CGroupBonding *first, pid_t pid);
+int cgroup_bonding_install(CGroupBonding *b, pid_t pid, const char *suffix);
+int cgroup_bonding_install_list(CGroupBonding *first, pid_t pid, const char *suffix);
 
 int cgroup_bonding_set_group_access(CGroupBonding *b, mode_t mode, uid_t uid, gid_t gid);
 int cgroup_bonding_set_group_access_list(CGroupBonding *b, mode_t mode, uid_t uid, gid_t gid);
@@ -65,8 +65,8 @@ int cgroup_bonding_set_group_access_list(CGroupBonding *b, mode_t mode, uid_t ui
 int cgroup_bonding_set_task_access(CGroupBonding *b, mode_t mode, uid_t uid, gid_t gid, int sticky);
 int cgroup_bonding_set_task_access_list(CGroupBonding *b, mode_t mode, uid_t uid, gid_t gid, int sticky);
 
-int cgroup_bonding_kill(CGroupBonding *b, int sig, bool sigcont, Set *s);
-int cgroup_bonding_kill_list(CGroupBonding *first, int sig, bool sigcont, Set *s);
+int cgroup_bonding_kill(CGroupBonding *b, int sig, bool sigcont, Set *s, const char *suffix);
+int cgroup_bonding_kill_list(CGroupBonding *first, int sig, bool sigcont, Set *s, const char *suffix);
 
 void cgroup_bonding_trim(CGroupBonding *first, bool delete_root);
 void cgroup_bonding_trim_list(CGroupBonding *first, bool delete_root);

diff --git a/src/core/execute.c b/src/core/execute.c
index 271c57f562186974fcde25e0c76b5afb99a8c5b8..c59f7e2daa63b3c50bfc8ecb081a2e9be9fb62ba 100644 (file)
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -962,6 +962,7 @@ int exec_spawn(ExecCommand *command,
                bool confirm_spawn,
                CGroupBonding *cgroup_bondings,
                CGroupAttribute *cgroup_attributes,
+               const char *cgroup_suffix,
                pid_t *ret) {
 
         pid_t pid;
@@ -1154,7 +1155,7 @@ int exec_spawn(ExecCommand *command,
                 }
 
                 if (cgroup_bondings) {
-                        err = cgroup_bonding_install_list(cgroup_bondings, 0);
+                        err = cgroup_bonding_install_list(cgroup_bondings, 0, cgroup_suffix);
                         if (err < 0) {
                                 r = EXIT_CGROUP;
                                 goto fail_child;
@@ -1505,7 +1506,7 @@ int exec_spawn(ExecCommand *command,
          * sure that when we kill the cgroup the process will be
          * killed too). */
         if (cgroup_bondings)
-                cgroup_bonding_install_list(cgroup_bondings, pid);
+                cgroup_bonding_install_list(cgroup_bondings, pid, cgroup_suffix);
 
         log_debug("Forked %s as %lu", command->path, (unsigned long) pid);
 

diff --git a/src/core/execute.h b/src/core/execute.h
index fc4c71e534c3d47695ae22125377cf9805f7bcd2..428bb7c9bfe0530ef39190f14351d5715841e109 100644 (file)
--- a/src/core/execute.h
+++ b/src/core/execute.h
@@ -192,6 +192,7 @@ int exec_spawn(ExecCommand *command,
                bool confirm_spawn,
                struct CGroupBonding *cgroup_bondings,
                struct CGroupAttribute *cgroup_attributes,
+               const char *cgroup_suffix,
                pid_t *ret);
 
 void exec_command_done(ExecCommand *c);

diff --git a/src/core/mount.c b/src/core/mount.c
index 6c768a3d0208647b203575bf66ecf1700a62c5cf..760ffcdbf6e983542828e876fc3a4a9ef1bd67ee 100644 (file)
--- a/src/core/mount.c
+++ b/src/core/mount.c
@@ -805,6 +805,7 @@ static int mount_spawn(Mount *m, ExecCommand *c, pid_t *_pid) {
                             UNIT(m)->manager->confirm_spawn,
                             UNIT(m)->cgroup_bondings,
                             UNIT(m)->cgroup_attributes,
+                            NULL,
                             &pid)) < 0)
                 goto fail;
 
@@ -875,7 +876,8 @@ static void mount_enter_signal(Mount *m, MountState state, MountResult f) {
                                 if ((r = set_put(pid_set, LONG_TO_PTR(m->control_pid))) < 0)
                                         goto fail;
 
-                        if ((r = cgroup_bonding_kill_list(UNIT(m)->cgroup_bondings, sig, true, pid_set)) < 0) {
+                        r = cgroup_bonding_kill_list(UNIT(m)->cgroup_bondings, sig, true, pid_set, NULL);
+                        if (r < 0) {
                                 if (r != -EAGAIN && r != -ESRCH && r != -ENOENT)
                                         log_warning("Failed to kill control group: %s", strerror(-r));
                         } else if (r > 0)
@@ -1833,7 +1835,8 @@ static int mount_kill(Unit *u, KillWho who, KillMode mode, int signo, DBusError
                                 goto finish;
                         }
 
-                if ((q = cgroup_bonding_kill_list(UNIT(m)->cgroup_bondings, signo, false, pid_set)) < 0)
+                q = cgroup_bonding_kill_list(UNIT(m)->cgroup_bondings, signo, false, pid_set, NULL);
+                if (q < 0)
                         if (q != -EAGAIN && q != -ESRCH && q != -ENOENT)
                                 r = q;
         }

diff --git a/src/core/service.c b/src/core/service.c
index 1c04ed338a8cc60c79324a9bf9c096d58ad4201b..59dd71294bcafef36df4a9c3f25e2708da7b0328 100644 (file)
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -1686,6 +1686,7 @@ static int service_spawn(
                 bool apply_chroot,
                 bool apply_tty_stdin,
                 bool set_notify_socket,
+                bool is_control,
                 pid_t *_pid) {
 
         pid_t pid;
@@ -1767,6 +1768,7 @@ static int service_spawn(
                        UNIT(s)->manager->confirm_spawn,
                        UNIT(s)->cgroup_bondings,
                        UNIT(s)->cgroup_attributes,
+                       is_control ? "control" : NULL,
                        &pid);
 
         if (r < 0)
@@ -1886,15 +1888,17 @@ static void service_enter_stop_post(Service *s, ServiceResult f) {
         if ((s->control_command = s->exec_command[SERVICE_EXEC_STOP_POST])) {
                 s->control_command_id = SERVICE_EXEC_STOP_POST;
 
-                if ((r = service_spawn(s,
-                                       s->control_command,
-                                       true,
-                                       false,
-                                       !s->permissions_start_only,
-                                       !s->root_directory_start_only,
-                                       true,
-                                       false,
-                                       &s->control_pid)) < 0)
+                r = service_spawn(s,
+                                  s->control_command,
+                                  true,
+                                  false,
+                                  !s->permissions_start_only,
+                                  !s->root_directory_start_only,
+                                  true,
+                                  false,
+                                  true,
+                                  &s->control_pid);
+                if (r < 0)
                         goto fail;
 
 
@@ -1952,7 +1956,8 @@ static void service_enter_signal(Service *s, ServiceState state, ServiceResult f
                                 if ((r = set_put(pid_set, LONG_TO_PTR(s->control_pid))) < 0)
                                         goto fail;
 
-                        if ((r = cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, sig, true, pid_set)) < 0) {
+                        r = cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, sig, true, pid_set, NULL);
+                        if (r < 0) {
                                 if (r != -EAGAIN && r != -ESRCH && r != -ENOENT)
                                         log_warning("Failed to kill control group: %s", strerror(-r));
                         } else if (r > 0)
@@ -2001,15 +2006,17 @@ static void service_enter_stop(Service *s, ServiceResult f) {
         if ((s->control_command = s->exec_command[SERVICE_EXEC_STOP])) {
                 s->control_command_id = SERVICE_EXEC_STOP;
 
-                if ((r = service_spawn(s,
-                                       s->control_command,
-                                       true,
-                                       false,
-                                       !s->permissions_start_only,
-                                       !s->root_directory_start_only,
-                                       false,
-                                       false,
-                                       &s->control_pid)) < 0)
+                r = service_spawn(s,
+                                  s->control_command,
+                                  true,
+                                  false,
+                                  !s->permissions_start_only,
+                                  !s->root_directory_start_only,
+                                  false,
+                                  false,
+                                  true,
+                                  &s->control_pid);
+                if (r < 0)
                         goto fail;
 
                 service_set_state(s, SERVICE_STOP);
@@ -2054,15 +2061,17 @@ static void service_enter_start_post(Service *s) {
         if ((s->control_command = s->exec_command[SERVICE_EXEC_START_POST])) {
                 s->control_command_id = SERVICE_EXEC_START_POST;
 
-                if ((r = service_spawn(s,
-                                       s->control_command,
-                                       true,
-                                       false,
-                                       !s->permissions_start_only,
-                                       !s->root_directory_start_only,
-                                       false,
-                                       false,
-                                       &s->control_pid)) < 0)
+                r = service_spawn(s,
+                                  s->control_command,
+                                  true,
+                                  false,
+                                  !s->permissions_start_only,
+                                  !s->root_directory_start_only,
+                                  false,
+                                  false,
+                                  true,
+                                  &s->control_pid);
+                if (r < 0)
                         goto fail;
 
                 service_set_state(s, SERVICE_START_POST);
@@ -2094,7 +2103,7 @@ static void service_enter_start(Service *s) {
         /* We want to ensure that nobody leaks processes from
          * START_PRE here, so let's go on a killing spree, People
          * should not spawn long running processes from START_PRE. */
-        cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, SIGKILL, true, NULL);
+        cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, SIGKILL, true, NULL, "control");
 
         if (s->type == SERVICE_FORKING) {
                 s->control_command_id = SERVICE_EXEC_START;
@@ -2108,15 +2117,17 @@ static void service_enter_start(Service *s) {
                 c = s->main_command = s->exec_command[SERVICE_EXEC_START];
         }
 
-        if ((r = service_spawn(s,
-                               c,
-                               s->type == SERVICE_FORKING || s->type == SERVICE_DBUS || s->type == SERVICE_NOTIFY,
-                               true,
-                               true,
-                               true,
-                               true,
-                               s->notify_access != NOTIFY_NONE,
-                               &pid)) < 0)
+        r = service_spawn(s,
+                          c,
+                          s->type == SERVICE_FORKING || s->type == SERVICE_DBUS || s->type == SERVICE_NOTIFY,
+                          true,
+                          true,
+                          true,
+                          true,
+                          s->notify_access != NOTIFY_NONE,
+                          false,
+                          &pid);
+        if (r < 0)
                 goto fail;
 
         if (s->type == SERVICE_SIMPLE) {
@@ -2168,19 +2179,21 @@ static void service_enter_start_pre(Service *s) {
 
                 /* Before we start anything, let's clear up what might
                  * be left from previous runs. */
-                cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, SIGKILL, true, NULL);
+                cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, SIGKILL, true, NULL, "control");
 
                 s->control_command_id = SERVICE_EXEC_START_PRE;
 
-                if ((r = service_spawn(s,
-                                       s->control_command,
-                                       true,
-                                       false,
-                                       !s->permissions_start_only,
-                                       !s->root_directory_start_only,
-                                       true,
-                                       false,
-                                       &s->control_pid)) < 0)
+                r = service_spawn(s,
+                                  s->control_command,
+                                  true,
+                                  false,
+                                  !s->permissions_start_only,
+                                  !s->root_directory_start_only,
+                                  true,
+                                  false,
+                                  true,
+                                  &s->control_pid);
+                if (r < 0)
                         goto fail;
 
                 service_set_state(s, SERVICE_START_PRE);
@@ -2236,15 +2249,17 @@ static void service_enter_reload(Service *s) {
         if ((s->control_command = s->exec_command[SERVICE_EXEC_RELOAD])) {
                 s->control_command_id = SERVICE_EXEC_RELOAD;
 
-                if ((r = service_spawn(s,
-                                       s->control_command,
-                                       true,
-                                       false,
-                                       !s->permissions_start_only,
-                                       !s->root_directory_start_only,
-                                       false,
-                                       false,
-                                       &s->control_pid)) < 0)
+                r = service_spawn(s,
+                                  s->control_command,
+                                  true,
+                                  false,
+                                  !s->permissions_start_only,
+                                  !s->root_directory_start_only,
+                                  false,
+                                  false,
+                                  true,
+                                  &s->control_pid);
+                if (r < 0)
                         goto fail;
 
                 service_set_state(s, SERVICE_RELOAD);
@@ -2271,16 +2286,18 @@ static void service_run_next_control(Service *s) {
         s->control_command = s->control_command->command_next;
         service_unwatch_control_pid(s);
 
-        if ((r = service_spawn(s,
-                               s->control_command,
-                               true,
-                               false,
-                               !s->permissions_start_only,
-                               !s->root_directory_start_only,
-                               s->control_command_id == SERVICE_EXEC_START_PRE ||
-                               s->control_command_id == SERVICE_EXEC_STOP_POST,
-                               false,
-                               &s->control_pid)) < 0)
+        r = service_spawn(s,
+                          s->control_command,
+                          true,
+                          false,
+                          !s->permissions_start_only,
+                          !s->root_directory_start_only,
+                          s->control_command_id == SERVICE_EXEC_START_PRE ||
+                          s->control_command_id == SERVICE_EXEC_STOP_POST,
+                          false,
+                          true,
+                          &s->control_pid);
+        if (r < 0)
                 goto fail;
 
         return;
@@ -2313,15 +2330,17 @@ static void service_run_next_main(Service *s) {
         s->main_command = s->main_command->command_next;
         service_unwatch_main_pid(s);
 
-        if ((r = service_spawn(s,
-                               s->main_command,
-                               false,
-                               true,
-                               true,
-                               true,
-                               true,
-                               s->notify_access != NOTIFY_NONE,
-                               &pid)) < 0)
+        r = service_spawn(s,
+                          s->main_command,
+                          false,
+                          true,
+                          true,
+                          true,
+                          true,
+                          s->notify_access != NOTIFY_NONE,
+                          false,
+                          &pid);
+        if (r < 0)
                 goto fail;
 
         service_set_main_pid(s, pid);
@@ -3647,8 +3666,8 @@ static int service_kill(Unit *u, KillWho who, KillMode mode, int signo, DBusErro
                                 r = q;
                                 goto finish;
                         }
-
-                if ((q = cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, signo, false, pid_set)) < 0)
+                q = cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, signo, false, pid_set, NULL);
+                if (q < 0)
                         if (q != -EAGAIN && q != -ESRCH && q != -ENOENT)
                                 r = q;
         }

diff --git a/src/core/socket.c b/src/core/socket.c
index 37a0236156992455c4ab5c09c7bf51e095902937..a439717651c8c362fc50fe75a0259e585ed74628 100644 (file)
--- a/src/core/socket.c
+++ b/src/core/socket.c
@@ -1150,6 +1150,7 @@ static int socket_spawn(Socket *s, ExecCommand *c, pid_t *_pid) {
                        UNIT(s)->manager->confirm_spawn,
                        UNIT(s)->cgroup_bondings,
                        UNIT(s)->cgroup_attributes,
+                       NULL,
                        &pid);
 
         strv_free(argv);
@@ -1240,7 +1241,8 @@ static void socket_enter_signal(Socket *s, SocketState state, SocketResult f) {
                                 if ((r = set_put(pid_set, LONG_TO_PTR(s->control_pid))) < 0)
                                         goto fail;
 
-                        if ((r = cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, sig, true, pid_set)) < 0) {
+                        r = cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, sig, true, pid_set, NULL);
+                        if (r < 0) {
                                 if (r != -EAGAIN && r != -ESRCH && r != -ENOENT)
                                         log_warning("Failed to kill control group: %s", strerror(-r));
                         } else if (r > 0)
@@ -2127,7 +2129,8 @@ static int socket_kill(Unit *u, KillWho who, KillMode mode, int signo, DBusError
                                 goto finish;
                         }
 
-                if ((q = cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, signo, false, pid_set)) < 0)
+                q = cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, signo, false, pid_set, NULL);
+                if (q < 0)
                         if (q != -EAGAIN && q != -ESRCH && q != -ENOENT)
                                 r = q;
         }

diff --git a/src/core/swap.c b/src/core/swap.c
index 6331864d8ca5b3f00520729f222b522cadc1604b..363852d33657f60493e6717971e7ee0e2100b9ad 100644 (file)
--- a/src/core/swap.c
+++ b/src/core/swap.c
@@ -621,6 +621,7 @@ static int swap_spawn(Swap *s, ExecCommand *c, pid_t *_pid) {
                             UNIT(s)->manager->confirm_spawn,
                             UNIT(s)->cgroup_bondings,
                             UNIT(s)->cgroup_attributes,
+                            NULL,
                             &pid)) < 0)
                 goto fail;
 
@@ -690,7 +691,8 @@ static void swap_enter_signal(Swap *s, SwapState state, SwapResult f) {
                                 if ((r = set_put(pid_set, LONG_TO_PTR(s->control_pid))) < 0)
                                         goto fail;
 
-                        if ((r = cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, sig, true, pid_set)) < 0) {
+                        r = cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, sig, true, pid_set, NULL);
+                        if (r < 0) {
                                 if (r != -EAGAIN && r != -ESRCH && r != -ENOENT)
                                         log_warning("Failed to kill control group: %s", strerror(-r));
                         } else if (r > 0)
@@ -1321,7 +1323,8 @@ static int swap_kill(Unit *u, KillWho who, KillMode mode, int signo, DBusError *
                                 goto finish;
                         }
 
-                if ((q = cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, signo, false, pid_set)) < 0)
+                q = cgroup_bonding_kill_list(UNIT(s)->cgroup_bondings, signo, false, pid_set, NULL);
+                if (q < 0)
                         if (q != -EAGAIN && q != -ESRCH && q != -ENOENT)
                                 r = q;
         }