From d068765b68f76a88dde70ca29173fa65c06f9f1b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 May 2020 13:59:04 +0200 Subject: [PATCH 1/9] core: warn if KillMode=none is used --- src/core/load-fragment.c | 43 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 352798fd6a..96a3f12103 100644 --- a/src/core/load-fragment.c +++ b/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, From 9b52e0d81af9c0fbdfc8686eecacc334f8d60aa8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 May 2020 14:08:09 +0200 Subject: [PATCH 2/9] man: document that KillMode=none is a bad idea --- man/systemd.kill.xml | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/man/systemd.kill.xml b/man/systemd.kill.xml index 6a1c67d406..5e4d430031 100644 --- a/man/systemd.kill.xml +++ b/man/systemd.kill.xml @@ -61,28 +61,25 @@ KillMode= - Specifies how processes of this unit shall be - killed. One of - , - , - , + Specifies how processes of this unit shall be killed. One of + , , , . - If set to , all remaining - processes in the control group of this unit will be killed on - unit stop (for services: after the stop command is executed, - as configured with ExecStop=). If set to - , only the main process itself is - killed. If set to , the - SIGTERM signal (see below) is sent to the - main process while the subsequent SIGKILL - signal (see below) is sent to all remaining processes of the - unit's control group. If set to , no - process is killed. In this case, only the stop command will be - executed on unit stop, but no process will be killed otherwise. - Processes remaining alive after stop are left in their control - group and the control group continues to exist after stop - unless it is empty. + If set to , all remaining processes in the control group of this + unit will be killed on unit stop (for services: after the stop command is executed, as configured + with ExecStop=). If set to , the + SIGTERM signal (see below) is sent to the main process while the subsequent + SIGKILL signal (see below) is sent to all remaining processes of the unit's + control group. If set to , only the main process itself is killed (not + recommended!). If set to , no process is killed (strongly recommended + against!). In this case, only the stop command will be executed on unit stop, but no process will be + killed otherwise. Processes remaining alive after stop are left in their control group and the + control group continues to exist after stop unless empty. + + Note that it is not recommended to set KillMode= to + process or even none, as this allows processes to escape + the service manager's lifecycle and resource management, and to remain running even while their + service is considered stopped and is assumed to not consume any resources. Processes will first be terminated via SIGTERM (unless the signal to send is changed via KillSignal= or RestartKillSignal=). Optionally, From 021acbc188a53fa528161578305406c5c9c808b2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 May 2020 14:12:56 +0200 Subject: [PATCH 3/9] units: remove KillMode=process in our regular gettys In our regular gettys the actual shell commands live the the session scope anyway (as long as logind is used). Hence, let's avoid KillMode=process, it serves no purpose and is simply unsafe since it disables systemd's own process lifecycle management. --- units/console-getty.service.m4 | 2 ++ units/container-getty@.service.m4 | 2 ++ units/getty@.service.m4 | 2 ++ units/serial-getty@.service.m4 | 2 ++ 4 files changed, 8 insertions(+) diff --git a/units/console-getty.service.m4 b/units/console-getty.service.m4 index 3c553240a2..7c3992e8e0 100644 --- a/units/console-getty.service.m4 +++ b/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 diff --git a/units/container-getty@.service.m4 b/units/container-getty@.service.m4 index 087ab7f9b1..e4f9ae5791 100644 --- a/units/container-getty@.service.m4 +++ b/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 diff --git a/units/getty@.service.m4 b/units/getty@.service.m4 index 80e793bb73..087d686666 100644 --- a/units/getty@.service.m4 +++ b/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 diff --git a/units/serial-getty@.service.m4 b/units/serial-getty@.service.m4 index 757b86ab2f..cab89fb426 100644 --- a/units/serial-getty@.service.m4 +++ b/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 From 4c42543429b5c167a7900423f04f8294d1ec4650 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 May 2020 14:29:46 +0200 Subject: [PATCH 4/9] core: also log about left-over processes during unit stop Only log at LOG_INFO level, i.e. make this informational. During start let's leave it at LOG_WARNING though. Of course, it's ugly leaving processes around like that either in start or in stop, but at start its more dangerous than on stop, so be tougher there. --- src/core/mount.c | 4 +++- src/core/service.c | 12 +++++++----- src/core/socket.c | 4 +++- src/core/swap.c | 3 ++- src/core/unit.c | 31 +++++++++++++++++++++++++++---- src/core/unit.h | 4 +++- 6 files changed, 45 insertions(+), 13 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 48500b4932..10613d11ae 100644 --- a/src/core/mount.c +++ b/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; diff --git a/src/core/service.c b/src/core/service.c index 6e2a82a286..3e955edeff 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1755,6 +1755,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."); @@ -2081,15 +2082,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 - * service where a SigKilled process would result in a lengthy recovery and who's shutdown or - * startup time is quite variable (so Timeout settings aren't of use). + * 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 startup + * 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), diff --git a/src/core/socket.c b/src/core/socket.c index 5e8f317bfc..218b4b245d 100644 --- a/src/core/socket.c +++ b/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]; diff --git a/src/core/swap.c b/src/core/swap.c index 0b42236aca..20179de2d2 100644 --- a/src/core/swap.c +++ b/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; diff --git a/src/core/unit.c b/src/core/unit.c index c5eb72163b..d55cbea68a 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -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) { diff --git a/src/core/unit.h b/src/core/unit.h index bee68607fe..6659406c2d 100644 --- a/src/core/unit.h +++ b/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); From b0cea477d4aebe8e1df2f91edc488245ab15f571 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 May 2020 14:31:34 +0200 Subject: [PATCH 5/9] core: some more structured initialization --- src/core/service.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 3e955edeff..aebb7778bc 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -447,14 +447,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->service = s; - fs->do_poll = do_poll; - fs->fdname = strdup(name ?: "stored"); + *fs = (ServiceFDStore) { + .fd = fd, + .service = s, + .do_poll = do_poll, + .fdname = strdup(name ?: "stored"), + }; + if (!fs->fdname) { free(fs); return -ENOMEM; From c8aa4b5b867a246acdffb1b11321647a487f27da Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 May 2020 14:32:03 +0200 Subject: [PATCH 6/9] core: voidify one function return --- src/core/unit.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index d55cbea68a..b287c77117 100644 --- a/src/core/unit.c +++ b/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, - SIGHUP, - CGROUP_IGNORE_SELF, - pid_set, - NULL, NULL); + (void) cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, + SIGHUP, + CGROUP_IGNORE_SELF, + pid_set, + NULL, NULL); } } } From 470ab28d07cdb2bb650e3bd2bb43aa5b9ac28b86 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 May 2020 14:47:05 +0200 Subject: [PATCH 7/9] sysv: use structured initialization --- src/sysv-generator/sysv-generator.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/sysv-generator/sysv-generator.c b/src/sysv-generator/sysv-generator.c index 5df5743823..a667a8ebad 100644 --- a/src/sysv-generator/sysv-generator.c +++ b/src/sysv-generator/sysv-generator.c @@ -788,19 +788,21 @@ static int enumerate_sysv(const LookupPaths *lp, Hashmap *all_services) { if (!fpath) return log_oom(); - service = new0(SysvStub, 1); + service = new(SysvStub, 1); if (!service) return log_oom(); - service->sysv_start_priority = -1; - service->name = TAKE_PTR(name); - service->path = TAKE_PTR(fpath); + *service = (SysvStub) { + .sysv_start_priority = -1, + .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); } } From 0e42cbe2541fefaa213eae249ef993a7cfb6367e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 May 2020 14:47:30 +0200 Subject: [PATCH 8/9] sysv: generate warning for every SysV service lacking a native systemd unit It's 2020, let's tighten the screws a bit and start warning about left-over SysV services that still have no native systemd unit file. --- src/sysv-generator/sysv-generator.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/sysv-generator/sysv-generator.c b/src/sysv-generator/sysv-generator.c index a667a8ebad..3ad139b242 100644 --- a/src/sysv-generator/sysv-generator.c +++ b/src/sysv-generator/sysv-generator.c @@ -788,6 +788,10 @@ static int enumerate_sysv(const LookupPaths *lp, Hashmap *all_services) { if (!fpath) return log_oom(); + 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(); From effefa30de46f25d0f50a36210a9835097381c2b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 26 May 2020 14:46:35 +0200 Subject: [PATCH 9/9] update TODO --- TODO | 2 ++ 1 file changed, 2 insertions(+) diff --git a/TODO b/TODO index cc7a332a8b..37323e3a19 100644 --- a/TODO +++ b/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