From a4634b214c59631763b2c8dbd012eb3dca8ac91a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 Nov 2017 22:02:22 +0100 Subject: [PATCH] core: warn about left-over processes in cgroup on unit start Now that we don't kill control processes anymore, let's at least warn about any processes left-over in the unit cgroup at the moment of starting the unit. --- src/core/cgroup.c | 42 +++++++++++++++++++++++++++++------------- src/core/cgroup.h | 1 + src/core/mount.c | 8 +++++--- src/core/service.c | 4 ++++ src/core/socket.c | 3 +++ src/core/swap.c | 2 ++ src/core/unit.c | 25 +++++++++++++++++++++++++ src/core/unit.h | 2 ++ 8 files changed, 71 insertions(+), 16 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 4208d1de810..78ef885b067 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1394,6 +1394,31 @@ int unit_watch_cgroup(Unit *u) { return 0; } +int unit_pick_cgroup_path(Unit *u) { + _cleanup_free_ char *path = NULL; + int r; + + assert(u); + + if (u->cgroup_path) + return 0; + + if (!UNIT_HAS_CGROUP_CONTEXT(u)) + return -EINVAL; + + path = unit_default_cgroup_path(u); + if (!path) + return log_oom(); + + r = unit_set_cgroup_path(u, path); + if (r == -EEXIST) + return log_unit_error_errno(u, r, "Control group %s exists already.", path); + if (r < 0) + return log_unit_error_errno(u, r, "Failed to set unit's control group path to %s: %m", path); + + return 0; +} + static int unit_create_cgroup( Unit *u, CGroupMask target_mask, @@ -1409,19 +1434,10 @@ static int unit_create_cgroup( if (!c) return 0; - if (!u->cgroup_path) { - _cleanup_free_ char *path = NULL; - - path = unit_default_cgroup_path(u); - if (!path) - return log_oom(); - - r = unit_set_cgroup_path(u, path); - if (r == -EEXIST) - return log_unit_error_errno(u, r, "Control group %s exists already.", path); - if (r < 0) - return log_unit_error_errno(u, r, "Failed to set unit's control group path to %s: %m", path); - } + /* Figure out our cgroup path */ + r = unit_pick_cgroup_path(u); + if (r < 0) + return r; /* First, create our own group */ r = cg_create_everywhere(u->manager->cgroup_supported, target_mask, u->cgroup_path); diff --git a/src/core/cgroup.h b/src/core/cgroup.h index 00df3bb3684..0c5bb4a2c8b 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -169,6 +169,7 @@ void unit_update_cgroup_members_masks(Unit *u); char *unit_default_cgroup_path(Unit *u); int unit_set_cgroup_path(Unit *u, const char *path); +int unit_pick_cgroup_path(Unit *u); int unit_realize_cgroup(Unit *u); void unit_release_cgroup(Unit *u); diff --git a/src/core/mount.c b/src/core/mount.c index 1500670b094..b25bb9cb403 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -938,9 +938,6 @@ static void mount_enter_mounting(Mount *m) { assert(m); - m->control_command_id = MOUNT_EXEC_MOUNT; - m->control_command = m->exec_command + MOUNT_EXEC_MOUNT; - r = unit_fail_if_symlink(UNIT(m), m->where); if (r < 0) goto fail; @@ -949,6 +946,11 @@ static void mount_enter_mounting(Mount *m) { unit_warn_if_dir_nonempty(UNIT(m), m->where); + unit_warn_leftover_processes(UNIT(m)); + + m->control_command_id = MOUNT_EXEC_MOUNT; + m->control_command = m->exec_command + MOUNT_EXEC_MOUNT; + /* Create the source directory for bind-mounts if needed */ p = get_mount_parameters_fragment(m); if (p && mount_is_bind(p)) diff --git a/src/core/service.c b/src/core/service.c index efd808784de..672eaa9199b 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1814,6 +1814,8 @@ static void service_enter_start(Service *s) { service_unwatch_control_pid(s); service_unwatch_main_pid(s); + unit_warn_leftover_processes(UNIT(s)); + if (s->type == SERVICE_FORKING) { s->control_command_id = SERVICE_EXEC_START; c = s->control_command = s->exec_command[SERVICE_EXEC_START]; @@ -1901,6 +1903,8 @@ static void service_enter_start_pre(Service *s) { s->control_command = s->exec_command[SERVICE_EXEC_START_PRE]; if (s->control_command) { + unit_warn_leftover_processes(UNIT(s)); + s->control_command_id = SERVICE_EXEC_START_PRE; r = service_spawn(s, diff --git a/src/core/socket.c b/src/core/socket.c index 572a70a2a8f..7e3630ada76 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -2187,6 +2187,9 @@ static void socket_enter_start_pre(Socket *s) { assert(s); socket_unwatch_control_pid(s); + + unit_warn_leftover_processes(UNIT(s)); + 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 115e59c5cab..849ccbdd431 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -734,6 +734,8 @@ static void swap_enter_activating(Swap *s) { assert(s); + unit_warn_leftover_processes(UNIT(s)); + 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 2ddd95c3ac6..856939853a8 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -5196,6 +5196,31 @@ int unit_prepare_exec(Unit *u) { return 0; } +static void log_leftover(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 */ + return; + + 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.", + pid, strna(comm)); +} + +void unit_warn_leftover_processes(Unit *u) { + assert(u); + + (void) unit_pick_cgroup_path(u); + + if (!u->cgroup_path) + return; + + (void) cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, 0, 0, NULL, log_leftover, u); +} + static const char* const collect_mode_table[_COLLECT_MODE_MAX] = { [COLLECT_INACTIVE] = "inactive", [COLLECT_INACTIVE_OR_FAILED] = "inactive-or-failed", diff --git a/src/core/unit.h b/src/core/unit.h index e33597c4b7f..cd7c08a20d8 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -768,6 +768,8 @@ void unit_unlink_state_files(Unit *u); int unit_prepare_exec(Unit *u); +void unit_warn_leftover_processes(Unit *u); + /* Macros which append UNIT= or USER_UNIT= to the message */ #define log_unit_full(unit, level, error, ...) \