Merge pull request #15928 from poettering/kill-mode-warnings

warn on KillMode=none, inform about left-over processes on stop and warn about sysv services

TODO

@@ -19,6 +19,8 @@ Janitorial Clean-ups:
 
 Features:
 
+* if /usr/bin/swapoff fails due to OOM, log a friendly explanatory message about it
+
 * build short web pages out of each catalog entry, build them along with man
   pages, and include hyperlinks to them in the journal output
 

man/systemd.kill.xml

@@ -61,28 +61,25 @@
 
       <varlistentry>
         <term><varname>KillMode=</varname></term>
-        <listitem><para>Specifies how processes of this unit shall be
+        <listitem><para>Specifies how processes of this unit shall be killed. One of
-        killed. One of
+        <option>control-group</option>, <option>mixed</option>, <option>process</option>,
-        <option>control-group</option>,
-        <option>process</option>,
-        <option>mixed</option>,
         <option>none</option>.</para>
 
-        <para>If set to <option>control-group</option>, all remaining
+        <para>If set to <option>control-group</option>, all remaining processes in the control group of this
-        processes in the control group of this unit will be killed on
+        unit will be killed on unit stop (for services: after the stop command is executed, as configured
-        unit stop (for services: after the stop command is executed,
+        with <varname>ExecStop=</varname>). If set to <option>mixed</option>, the
-        as configured with <varname>ExecStop=</varname>). If set to
+        <constant>SIGTERM</constant> signal (see below) is sent to the main process while the subsequent
-        <option>process</option>, only the main process itself is
+        <constant>SIGKILL</constant> signal (see below) is sent to all remaining processes of the unit's
-        killed. If set to <option>mixed</option>, the
+        control group. If set to <option>process</option>, only the main process itself is killed (not
-        <constant>SIGTERM</constant> signal (see below) is sent to the
+        recommended!). If set to <option>none</option>, no process is killed (strongly recommended
-        main process while the subsequent <constant>SIGKILL</constant>
+        against!). In this case, only the stop command will be executed on unit stop, but no process will be
-        signal (see below) is sent to all remaining processes of the
+        killed otherwise.  Processes remaining alive after stop are left in their control group and the
-        unit's control group. If set to <option>none</option>, no
+        control group continues to exist after stop unless empty.</para>
-        process is killed. In this case, only the stop command will be
+
-        executed on unit stop, but no process will be killed otherwise.
+        <para>Note that it is not recommended to set <varname>KillMode=</varname> to
-        Processes remaining alive after stop are left in their control
+        <constant>process</constant> or even <constant>none</constant>, as this allows processes to escape
-        group and the control group continues to exist after stop
+        the service manager's lifecycle and resource management, and to remain running even while their
-        unless it is empty.</para>
+        service is considered stopped and is assumed to not consume any resources.</para>
 
         <para>Processes will first be terminated via <constant>SIGTERM</constant> (unless the signal to send
         is changed via <varname>KillSignal=</varname> or <varname>RestartKillSignal=</varname>). Optionally,

src/core/load-fragment.c

@@ -117,7 +117,6 @@ DEFINE_CONFIG_PARSE_ENUM(config_parse_device_policy, cgroup_device_policy, CGrou
 DEFINE_CONFIG_PARSE_ENUM(config_parse_exec_keyring_mode, exec_keyring_mode, ExecKeyringMode, "Failed to parse keyring mode");
 DEFINE_CONFIG_PARSE_ENUM(config_parse_exec_utmp_mode, exec_utmp_mode, ExecUtmpMode, "Failed to parse utmp mode");
 DEFINE_CONFIG_PARSE_ENUM(config_parse_job_mode, job_mode, JobMode, "Failed to parse job mode");
-DEFINE_CONFIG_PARSE_ENUM(config_parse_kill_mode, kill_mode, KillMode, "Failed to parse kill mode");
 DEFINE_CONFIG_PARSE_ENUM(config_parse_notify_access, notify_access, NotifyAccess, "Failed to parse notify access specifier");
 DEFINE_CONFIG_PARSE_ENUM(config_parse_protect_home, protect_home, ProtectHome, "Failed to parse protect home value");
 DEFINE_CONFIG_PARSE_ENUM(config_parse_protect_system, protect_system, ProtectSystem, "Failed to parse protect system value");
@@ -631,6 +630,48 @@ int config_parse_exec_coredump_filter(
         return 0;
 }
 
+int config_parse_kill_mode(
+                const char* unit,
+                const char *filename,
+                unsigned line,
+                const char *section,
+                unsigned section_line,
+                const char *lvalue,
+                int ltype,
+                const char *rvalue,
+                void *data,
+                void *userdata) {
+
+        KillMode *k = data, m;
+
+        assert(filename);
+        assert(lvalue);
+        assert(rvalue);
+        assert(data);
+
+        if (isempty(rvalue)) {
+                *k = KILL_CONTROL_GROUP;
+                return 0;
+        }
+
+        m = kill_mode_from_string(rvalue);
+        if (m < 0) {
+                log_syntax(unit, LOG_WARNING, filename, line, 0,
+                           "Failed to parse kill mode specification, ignoring: %s", rvalue);
+                return 0;
+        }
+
+        if (m == KILL_NONE)
+                log_syntax(unit, LOG_WARNING, filename, line, 0,
+                           "Unit configured to use KillMode=none. "
+                           "This is unsafe, as it disables systemd's process life-cycle management for the service. "
+                           "Please update your service to use a safer KillMode=, such as 'mixed' or 'control-group'. "
+                           "Support for KillMode=none is deprecated and will eventually be removed.");
+
+        *k = m;
+        return 0;
+}
+
 int config_parse_exec(
                 const char *unit,
                 const char *filename,

src/core/mount.c

@@ -863,6 +863,8 @@ static void mount_enter_dead(Mount *m, MountResult f) {
                 m->result = f;
 
         unit_log_result(UNIT(m), m->result == MOUNT_SUCCESS, mount_result_to_string(m->result));
+        unit_warn_leftover_processes(UNIT(m), unit_log_leftover_process_stop);
+
         mount_set_state(m, m->result != MOUNT_SUCCESS ? MOUNT_FAILED : MOUNT_DEAD);
 
         m->exec_runtime = exec_runtime_unref(m->exec_runtime, true);
@@ -1008,7 +1010,7 @@ static void mount_enter_mounting(Mount *m) {
         (void) mkdir_p_label(m->where, m->directory_mode);
 
         unit_warn_if_dir_nonempty(UNIT(m), m->where);
-        unit_warn_leftover_processes(UNIT(m));
+        unit_warn_leftover_processes(UNIT(m), unit_log_leftover_process_start);
 
         m->control_command_id = MOUNT_EXEC_MOUNT;
         m->control_command = m->exec_command + MOUNT_EXEC_MOUNT;

src/core/service.c

@@ -438,14 +438,17 @@ static int service_add_fd_store(Service *s, int fd, const char *name, bool do_po
                 }
         }
 
-        fs = new0(ServiceFDStore, 1);
+        fs = new(ServiceFDStore, 1);
         if (!fs)
                 return -ENOMEM;
 
-        fs->fd = fd;
+        *fs = (ServiceFDStore) {
-        fs->service = s;
+                .fd = fd,
-        fs->do_poll = do_poll;
+                .service = s,
-        fs->fdname = strdup(name ?: "stored");
+                .do_poll = do_poll,
+                .fdname = strdup(name ?: "stored"),
+        };
+
         if (!fs->fdname) {
                 free(fs);
                 return -ENOMEM;
@@ -1746,6 +1749,7 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart)
                 unit_log_failure(UNIT(s), service_result_to_string(s->result));
                 end_state = SERVICE_FAILED;
         }
+        unit_warn_leftover_processes(UNIT(s), unit_log_leftover_process_stop);
 
         if (!allow_restart)
                 log_unit_debug(UNIT(s), "Service restart not allowed.");
@@ -2072,15 +2076,16 @@ static int service_adverse_to_leftover_processes(Service *s) {
         assert(s);
 
         /* KillMode=mixed and control group are used to indicate that all process should be killed off.
-         * SendSIGKILL is used for services that require a clean shutdown. These are typically database
+         * SendSIGKILL= is used for services that require a clean shutdown. These are typically database
-         * service where a SigKilled process would result in a lengthy recovery and who's shutdown or
+         * service where a SigKilled process would result in a lengthy recovery and who's shutdown or startup
-         * startup time is quite variable (so Timeout settings aren't of use).
+         * time is quite variable (so Timeout settings aren't of use).
          *
          * Here we take these two factors and refuse to start a service if there are existing processes
          * within a control group. Databases, while generally having some protection against multiple
-         * instances running, lets not stress the rigor of these. Also ExecStartPre parts of the service
+         * instances running, lets not stress the rigor of these. Also ExecStartPre= parts of the service
          * aren't as rigoriously written to protect aganst against multiple use. */
-        if (unit_warn_leftover_processes(UNIT(s)) &&
+
+        if (unit_warn_leftover_processes(UNIT(s), unit_log_leftover_process_start) > 0 &&
             IN_SET(s->kill_context.kill_mode, KILL_MIXED, KILL_CONTROL_GROUP) &&
             !s->kill_context.send_sigkill)
                return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(EBUSY),

src/core/socket.c

@@ -2035,6 +2035,8 @@ static void socket_enter_dead(Socket *s, SocketResult f) {
         else
                 unit_log_failure(UNIT(s), socket_result_to_string(s->result));
 
+        unit_warn_leftover_processes(UNIT(s), unit_log_leftover_process_stop);
+
         socket_set_state(s, s->result != SOCKET_SUCCESS ? SOCKET_FAILED : SOCKET_DEAD);
 
         s->exec_runtime = exec_runtime_unref(s->exec_runtime, true);
@@ -2237,7 +2239,7 @@ static void socket_enter_start_pre(Socket *s) {
 
         socket_unwatch_control_pid(s);
 
-        unit_warn_leftover_processes(UNIT(s));
+        unit_warn_leftover_processes(UNIT(s), unit_log_leftover_process_start);
 
         s->control_command_id = SOCKET_EXEC_START_PRE;
         s->control_command = s->exec_command[SOCKET_EXEC_START_PRE];

src/core/swap.c

@@ -701,6 +701,7 @@ static void swap_enter_dead(Swap *s, SwapResult f) {
                 s->result = f;
 
         unit_log_result(UNIT(s), s->result == SWAP_SUCCESS, swap_result_to_string(s->result));
+        unit_warn_leftover_processes(UNIT(s), unit_log_leftover_process_stop);
         swap_set_state(s, s->result != SWAP_SUCCESS ? SWAP_FAILED : SWAP_DEAD);
 
         s->exec_runtime = exec_runtime_unref(s->exec_runtime, true);
@@ -788,7 +789,7 @@ static void swap_enter_activating(Swap *s) {
 
         assert(s);
 
-        unit_warn_leftover_processes(UNIT(s));
+        unit_warn_leftover_processes(UNIT(s), unit_log_leftover_process_start);
 
         s->control_command_id = SWAP_EXEC_ACTIVATE;
         s->control_command = s->exec_command + SWAP_EXEC_ACTIVATE;

src/core/unit.c

@@ -4971,11 +4971,11 @@ int unit_kill_context(
                                 if (!pid_set)
                                         return -ENOMEM;
 
-                                cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path,
+                                (void) cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path,
-                                                  SIGHUP,
+                                                         SIGHUP,
-                                                  CGROUP_IGNORE_SELF,
+                                                         CGROUP_IGNORE_SELF,
-                                                  pid_set,
+                                                         pid_set,
-                                                  NULL, NULL);
+                                                         NULL, NULL);
                         }
                 }
         }
@@ -5861,14 +5861,20 @@ int unit_prepare_exec(Unit *u) {
         return 0;
 }
 
-static int log_leftover(pid_t pid, int sig, void *userdata) {
+static bool ignore_leftover_process(const char *comm) {
+        return comm && comm[0] == '('; /* Most likely our own helper process (PAM?), ignore */
+}
+
+int unit_log_leftover_process_start(pid_t pid, int sig, void *userdata) {
         _cleanup_free_ char *comm = NULL;
 
         (void) get_process_comm(pid, &comm);
 
-        if (comm && comm[0] == '(') /* Most likely our own helper process (PAM?), ignore */
+        if (ignore_leftover_process(comm))
                 return 0;
 
+        /* During start we print a warning */
+
         log_unit_warning(userdata,
                          "Found left-over process " PID_FMT " (%s) in control group while starting unit. Ignoring.\n"
                          "This usually indicates unclean termination of a previous run, or service implementation deficiencies.",
@@ -5877,7 +5883,24 @@ static int log_leftover(pid_t pid, int sig, void *userdata) {
         return 1;
 }
 
-int unit_warn_leftover_processes(Unit *u) {
+int unit_log_leftover_process_stop(pid_t pid, int sig, void *userdata) {
+        _cleanup_free_ char *comm = NULL;
+
+        (void) get_process_comm(pid, &comm);
+
+        if (ignore_leftover_process(comm))
+                return 0;
+
+        /* During stop we only print an informational message */
+
+        log_unit_info(userdata,
+                      "Unit process " PID_FMT " (%s) remains running after unit stopped.",
+                      pid, strna(comm));
+
+        return 1;
+}
+
+int unit_warn_leftover_processes(Unit *u, cg_kill_log_func_t log_func) {
         assert(u);
 
         (void) unit_pick_cgroup_path(u);
@@ -5885,7 +5908,7 @@ int unit_warn_leftover_processes(Unit *u) {
         if (!u->cgroup_path)
                 return 0;
 
-        return cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, 0, 0, NULL, log_leftover, u);
+        return cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, 0, 0, NULL, log_func, u);
 }
 
 bool unit_needs_console(Unit *u) {

src/core/unit.h

@@ -846,7 +846,9 @@ void unit_unlink_state_files(Unit *u);
 
 int unit_prepare_exec(Unit *u);
 
-int unit_warn_leftover_processes(Unit *u);
+int unit_log_leftover_process_start(pid_t pid, int sig, void *userdata);
+int unit_log_leftover_process_stop(pid_t pid, int sig, void *userdata);
+int unit_warn_leftover_processes(Unit *u, cg_kill_log_func_t log_func);
 
 bool unit_needs_console(Unit *u);
 

src/sysv-generator/sysv-generator.c

@@ -788,19 +788,25 @@ static int enumerate_sysv(const LookupPaths *lp, Hashmap *all_services) {
                         if (!fpath)
                                 return log_oom();
 
-                        service = new0(SysvStub, 1);
+                        log_warning("SysV service '%s' lacks a native systemd unit file. "
+                                    "Automatically generating a unit file for compatibility. "
+                                    "Please update package to include a native systemd unit file, in order to make it more safe and robust.", fpath);
+
+                        service = new(SysvStub, 1);
                         if (!service)
                                 return log_oom();
 
-                        service->sysv_start_priority = -1;
+                        *service = (SysvStub) {
-                        service->name = TAKE_PTR(name);
+                                .sysv_start_priority = -1,
-                        service->path = TAKE_PTR(fpath);
+                                .name = TAKE_PTR(name),
+                                .path = TAKE_PTR(fpath),
+                        };
 
                         r = hashmap_put(all_services, service->name, service);
                         if (r < 0)
                                 return log_oom();
 
-                        service = NULL;
+                        TAKE_PTR(service);
                 }
         }
 

units/console-getty.service.m4

@@ -30,7 +30,9 @@ UtmpIdentifier=cons
 TTYPath=/dev/console
 TTYReset=yes
 TTYVHangup=yes
+m4_ifdef(`ENABLE_LOGIND',,
 KillMode=process
+)m4_dnl
 IgnoreSIGPIPE=no
 SendSIGHUP=yes
 

units/container-getty@.service.m4

@@ -36,6 +36,8 @@ UtmpIdentifier=pts/%I
 TTYPath=/dev/pts/%I
 TTYReset=yes
 TTYVHangup=yes
+m4_ifdef(`ENABLE_LOGIND',,
 KillMode=process
+)m4_dnl
 IgnoreSIGPIPE=no
 SendSIGHUP=yes

units/getty@.service.m4

@@ -47,7 +47,9 @@ TTYPath=/dev/%I
 TTYReset=yes
 TTYVHangup=yes
 TTYVTDisallocate=yes
+m4_ifdef(`ENABLE_LOGIND',,
 KillMode=process
+)m4_dnl
 IgnoreSIGPIPE=no
 SendSIGHUP=yes
 

units/serial-getty@.service.m4

@@ -40,7 +40,9 @@ UtmpIdentifier=%I
 TTYPath=/dev/%I
 TTYReset=yes
 TTYVHangup=yes
+m4_ifdef(`ENABLE_LOGIND',,
 KillMode=process
+)m4_dnl
 IgnoreSIGPIPE=no
 SendSIGHUP=yes