From deb3b156acd561d80d0f612e259cea4952d11e2b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 Nov 2017 18:29:35 +0100 Subject: [PATCH 1/9] mkosi: let's switch to the unified mode for mkosi This is a toolconfiguration for developers, and hence most likely should be the first thing to be switched over. Do so. --- mkosi.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mkosi.build b/mkosi.build index 71251920ed..7a72b63b94 100755 --- a/mkosi.build +++ b/mkosi.build @@ -28,7 +28,7 @@ export LC_CTYPE=en_US.UTF-8 sysvinit_path=`realpath /etc/init.d` -[ -f "$BUILDDIR"/build.ninja ] || meson "$BUILDDIR" -D "sysvinit-path=$sysvinit_path" +[ -f "$BUILDDIR"/build.ninja ] || meson "$BUILDDIR" -D "sysvinit-path=$sysvinit_path" -D "default-hierarchy=unified" ninja -C "$BUILDDIR" all [ "$WITH_TESTS" = 0 ] || ninja -C "$BUILDDIR" test || ( RET="$?" ; cat "$BUILDDIR"/meson-logs/testlog.txt ; exit "$RET" ) ninja -C "$BUILDDIR" install From 62b9bb2661cf640a541e8a817f73ed57c3f1d37f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 Nov 2017 18:30:23 +0100 Subject: [PATCH 2/9] cgroup-util: merge cg_set_tasks_access() and cg-set_group_access() into one We never use these functions seperately, hence don't bother splitting them into to. Also, simplify things a bit, and maintain tables for the attribute files to chown. Let's also update those tables a bit, and include thenew "cgroup.threads" file in it, that needs to be delegated too, according to the documentation. --- src/basic/cgroup-util.c | 144 ++++++++++++++++------------------------ src/basic/cgroup-util.h | 3 +- src/core/execute.c | 15 ++--- 3 files changed, 64 insertions(+), 98 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 7e4670ea8c..5774d4dea1 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -876,115 +876,87 @@ int cg_attach_fallback(const char *controller, const char *path, pid_t pid) { return r; } -int cg_set_group_access( +int cg_set_access( const char *controller, const char *path, - mode_t mode, uid_t uid, gid_t gid) { - _cleanup_free_ char *fs = NULL; - int r; + struct Attribute { + const char *name; + bool fatal; + }; - if (mode == MODE_INVALID && uid == UID_INVALID && gid == GID_INVALID) + /* cgroupsv1, aka legacy/non-unified */ + static const struct Attribute legacy_attributes[] = { + { "cgroup.procs", true }, + { "tasks", false }, + { "cgroup.clone_children", false }, + {}, + }; + + /* cgroupsv2, aka unified */ + static const struct Attribute unified_attributes[] = { + { "cgroup.procs", true }, + { "cgroup.subtree_control", true }, + { "cgroup.threads", false }, + {}, + }; + + static const struct Attribute* const attributes[] = { + [false] = legacy_attributes, + [true] = unified_attributes, + }; + + _cleanup_free_ char *fs = NULL; + const struct Attribute *i; + int r, unified; + + assert(path); + + if (uid == UID_INVALID && gid == GID_INVALID) return 0; - if (mode != MODE_INVALID) - mode &= 0777; + unified = cg_unified_controller(controller); + if (unified < 0) + return unified; + /* Configure access to the cgroup itself */ r = cg_get_path(controller, path, NULL, &fs); if (r < 0) return r; - r = chmod_and_chown(fs, mode, uid, gid); + r = chmod_and_chown(fs, 0755, uid, gid); if (r < 0) return r; - r = cg_hybrid_unified(); - if (r < 0) - return r; - if (r > 0 && streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { - r = cg_set_group_access(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path, mode, uid, gid); - if (r < 0) - log_debug_errno(r, "Failed to set group access on compatibility systemd cgroup %s, ignoring: %m", path); - } - - return 0; -} - -int cg_set_task_access( - const char *controller, - const char *path, - mode_t mode, - uid_t uid, - gid_t gid) { - - _cleanup_free_ char *fs = NULL; - int r; - - assert(path); - - if (mode == MODE_INVALID && uid == UID_INVALID && gid == GID_INVALID) - return 0; - - if (mode != MODE_INVALID) - mode &= 0666; - - /* For both the legacy and unified hierarchies, "cgroup.procs" is the main entry point for PIDs */ - r = cg_get_path(controller, path, "cgroup.procs", &fs); - if (r < 0) - return r; - - r = chmod_and_chown(fs, mode, uid, gid); - if (r < 0) - return r; - - r = cg_unified_controller(controller); - if (r < 0) - return r; - if (r == 0) { - const char *fn; - - /* Compatibility: on cgroupsv1 always keep values for the legacy files "tasks" and - * "cgroup.clone_children" in sync with "cgroup.procs". Since this is legacy stuff, we don't care if - * this fails. */ - - FOREACH_STRING(fn, - "tasks", - "cgroup.clone_children") { - - fs = mfree(fs); - - r = cg_get_path(controller, path, fn, &fs); - if (r < 0) - log_debug_errno(r, "Failed to get path for %s of %s, ignoring: %m", fn, path); - - r = chmod_and_chown(fs, mode, uid, gid); - if (r < 0) - log_debug_errno(r, "Failed to to change ownership/access mode for %s of %s, ignoring: %m", fn, path); - } - } else { - /* On the unified controller, we want to permit subtree controllers too. */ - + /* Configure access to the cgroup's attributes */ + for (i = attributes[unified]; i->name; i++) { fs = mfree(fs); - r = cg_get_path(controller, path, "cgroup.subtree_control", &fs); + + r = cg_get_path(controller, path, i->name, &fs); if (r < 0) return r; - r = chmod_and_chown(fs, mode, uid, gid); - if (r < 0) - return r; + r = chmod_and_chown(fs, 0644, uid, gid); + if (r < 0) { + if (i->fatal) + return r; + + log_debug_errno(r, "Failed to set access on cgroup %s, ignoring: %m", fs); + } } - r = cg_hybrid_unified(); - if (r < 0) - return r; - if (r > 0 && streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { - /* Always propagate access mode from unified to legacy controller */ - - r = cg_set_task_access(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path, mode, uid, gid); + if (streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { + r = cg_hybrid_unified(); if (r < 0) - log_debug_errno(r, "Failed to set task access on compatibility systemd cgroup %s, ignoring: %m", path); + return r; + if (r > 0) { + /* Always propagate access mode from unified to legacy controller */ + r = cg_set_access(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path, uid, gid); + if (r < 0) + log_debug_errno(r, "Failed to set access on compatibility systemd cgroup %s, ignoring: %m", path); + } } return 0; diff --git a/src/basic/cgroup-util.h b/src/basic/cgroup-util.h index e286e4e9c2..05c9f84505 100644 --- a/src/basic/cgroup-util.h +++ b/src/basic/cgroup-util.h @@ -188,8 +188,7 @@ int cg_set_attribute(const char *controller, const char *path, const char *attri int cg_get_attribute(const char *controller, const char *path, const char *attribute, char **ret); int cg_get_keyed_attribute(const char *controller, const char *path, const char *attribute, const char **keys, char **values); -int cg_set_group_access(const char *controller, const char *path, mode_t mode, uid_t uid, gid_t gid); -int cg_set_task_access(const char *controller, const char *path, mode_t mode, uid_t uid, gid_t gid); +int cg_set_access(const char *controller, const char *path, uid_t uid, gid_t gid); int cg_set_xattr(const char *controller, const char *path, const char *name, const void *value, size_t size, int flags); int cg_get_xattr(const char *controller, const char *path, const char *name, void *value, size_t size); diff --git a/src/core/execute.c b/src/core/execute.c index 9bdcb1abbf..2b936bcf4a 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -3009,17 +3009,12 @@ static int exec_child( } } - /* If delegation is enabled we'll pass ownership of the cgroup - * (but only in systemd's own controller hierarchy!) to the - * user of the new process. */ + /* If delegation is enabled we'll pass ownership of the cgroup to the user of the new process. On cgroupsv1 + * this is only about systemd's own hierarchy, i.e. not the controller hierarchies, simply because that's not + * safe. On cgroupsv2 there's only one hierarchy anyway, and delegation is safe there, hence in that case only + * touch a single hierarchy too. */ if (params->cgroup_path && context->user && (params->flags & EXEC_CGROUP_DELEGATE)) { - r = cg_set_task_access(SYSTEMD_CGROUP_CONTROLLER, params->cgroup_path, 0644, uid, gid); - if (r < 0) { - *exit_status = EXIT_CGROUP; - return log_unit_error_errno(unit, r, "Failed to adjust control group access: %m"); - } - - r = cg_set_group_access(SYSTEMD_CGROUP_CONTROLLER, params->cgroup_path, 0755, uid, gid); + r = cg_set_access(SYSTEMD_CGROUP_CONTROLLER, params->cgroup_path, uid, gid); if (r < 0) { *exit_status = EXIT_CGROUP; return log_unit_error_errno(unit, r, "Failed to adjust control group access: %m"); From 0f2d84d2ccda91bb1c1d9a5f24bfeab136e195eb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 Nov 2017 19:37:01 +0100 Subject: [PATCH 3/9] cgroup: drop unused parameter from function --- src/core/cgroup.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index ec8d89f0c2..8ae63e8615 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -677,9 +677,11 @@ static void cgroup_apply_unified_memory_limit(Unit *u, const char *file, uint64_ "Failed to set %s: %m", file); } -static void cgroup_apply_firewall(Unit *u, CGroupContext *c) { +static void cgroup_apply_firewall(Unit *u) { int r; + assert(u); + if (u->type == UNIT_SLICE) /* Skip this for slice units, they are inner cgroup nodes, and since bpf/cgroup is * not recursive we don't ever touch the bpf on them */ return; @@ -1031,7 +1033,7 @@ static void cgroup_context_apply( } if (apply_bpf) - cgroup_apply_firewall(u, c); + cgroup_apply_firewall(u); } CGroupMask cgroup_context_get_mask(CGroupContext *c) { From 2aa57a655004c8f9fba48eeb71aa9a31fdf3c94b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 Nov 2017 19:48:38 +0100 Subject: [PATCH 4/9] cgroup: when dispatching the cgroup realization queue, check again if we shall actually realize We add units to the cgroup realization queue when propagating realizing requests to sibling units, and when invalidating cgroup settings because some cgroup setting changed. In the time between where we add the unit to the queue until the cgroup is actually dispatched the unit's state might have changed however, so that the unit doesn't actually need to be realized anymore, for example because the unit went down. To handle that, check the unit state again, if realization makes sense. Redundant realization is usually not a problem, except when the unit is not actually running, hence check exactly for that. --- src/core/cgroup.c | 43 ++++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 8ae63e8615..51c52ccabe 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1505,6 +1505,27 @@ static bool unit_has_mask_realized( (!needs_bpf && u->cgroup_bpf_state == UNIT_CGROUP_BPF_OFF)); } +static void unit_add_to_cgroup_realize_queue(Unit *u) { + assert(u); + + if (u->in_cgroup_realize_queue) + return; + + LIST_PREPEND(cgroup_realize_queue, u->manager->cgroup_realize_queue, u); + u->in_cgroup_realize_queue = true; +} + +static void unit_remove_from_cgroup_realize_queue(Unit *u) { + assert(u); + + if (!u->in_cgroup_realize_queue) + return; + + LIST_REMOVE(cgroup_realize_queue, u->manager->cgroup_realize_queue, u); + u->in_cgroup_realize_queue = false; +} + + /* Check if necessary controllers and attributes for a unit are in place. * * If so, do nothing. @@ -1518,10 +1539,7 @@ static int unit_realize_cgroup_now(Unit *u, ManagerState state) { assert(u); - if (u->in_cgroup_realize_queue) { - LIST_REMOVE(cgroup_realize_queue, u->manager->cgroup_realize_queue, u); - u->in_cgroup_realize_queue = false; - } + unit_remove_from_cgroup_realize_queue(u); target_mask = unit_get_target_mask(u); enable_mask = unit_get_enable_mask(u); @@ -1554,16 +1572,6 @@ static int unit_realize_cgroup_now(Unit *u, ManagerState state) { return 0; } -static void unit_add_to_cgroup_realize_queue(Unit *u) { - assert(u); - - if (u->in_cgroup_realize_queue) - return; - - LIST_PREPEND(cgroup_realize_queue, u->manager->cgroup_realize_queue, u); - u->in_cgroup_realize_queue = true; -} - unsigned manager_dispatch_cgroup_realize_queue(Manager *m) { ManagerState state; unsigned n = 0; @@ -1577,6 +1585,12 @@ unsigned manager_dispatch_cgroup_realize_queue(Manager *m) { while ((i = m->cgroup_realize_queue)) { assert(i->in_cgroup_realize_queue); + if (UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(i))) { + /* Maybe things changed, and the unit is not actually active anymore? */ + unit_remove_from_cgroup_realize_queue(i); + continue; + } + r = unit_realize_cgroup_now(i, state); if (r < 0) log_warning_errno(r, "Failed to realize cgroups for queued unit %s, ignoring: %m", i->id); @@ -2353,7 +2367,6 @@ int unit_get_ip_accounting( fd = IN_SET(metric, CGROUP_IP_INGRESS_BYTES, CGROUP_IP_INGRESS_PACKETS) ? u->ip_accounting_ingress_map_fd : u->ip_accounting_egress_map_fd; - if (fd < 0) return -ENODATA; From 60c728adf7e716ca23e2d300d3955c60265a28c6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 Nov 2017 19:51:36 +0100 Subject: [PATCH 5/9] unit: initialize bpf cgroup realization state properly Before this patch, the bpf cgroup realization state was implicitly set to "NO", meaning that the bpf configuration was realized but was turned off. That means invalidation requests for the bpf stuff (which we issue in blanket fashion when doing a daemon reload) would actually later result in a us re-realizing the unit, under the assumption it was already realized once, even though in reality it never was realized before. This had the effect that after each daemon-reload we'd end up realizing *all* defined units, even the unloaded ones, populating cgroupfs with lots of unneeded empty cgroups. With this fix we properly set the realiazation state to "INVALIDATED", i.e. indicating the bpf stuff was never set up for the unit, and hence when we try to invalidate it later we won't do anything. --- src/core/cgroup.c | 4 ++-- src/core/unit.c | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 51c52ccabe..4208d1de81 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -2436,7 +2436,7 @@ void unit_invalidate_cgroup(Unit *u, CGroupMask m) { if (m & (CGROUP_MASK_CPU | CGROUP_MASK_CPUACCT)) m |= CGROUP_MASK_CPU | CGROUP_MASK_CPUACCT; - if ((u->cgroup_realized_mask & m) == 0) + if ((u->cgroup_realized_mask & m) == 0) /* NOP? */ return; u->cgroup_realized_mask &= ~m; @@ -2449,7 +2449,7 @@ void unit_invalidate_cgroup_bpf(Unit *u) { if (!UNIT_HAS_CGROUP_CONTEXT(u)) return; - if (u->cgroup_bpf_state == UNIT_CGROUP_BPF_INVALIDATED) + if (u->cgroup_bpf_state == UNIT_CGROUP_BPF_INVALIDATED) /* NOP? */ return; u->cgroup_bpf_state = UNIT_CGROUP_BPF_INVALIDATED; diff --git a/src/core/unit.c b/src/core/unit.c index c419caef10..a5feb11ddb 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -108,6 +108,7 @@ Unit *unit_new(Manager *m, size_t size) { u->ref_uid = UID_INVALID; u->ref_gid = GID_INVALID; u->cpu_usage_last = NSEC_INFINITY; + u->cgroup_bpf_state = UNIT_CGROUP_BPF_INVALIDATED; u->ip_accounting_ingress_map_fd = -1; u->ip_accounting_egress_map_fd = -1; From e9a4f67609cfabfeccbda8565bc4daf5b9e45c54 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 Nov 2017 20:11:27 +0100 Subject: [PATCH 6/9] cgroup: remove logic for maintaining /control subcgroup for the service unit type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, in the service unit type we ran all control processes in a special subcgroup /control of the unit's main cgroup. Remove that, and run the control program in the main cgroup instead. The concept conflicts with cgroupv2's logic of "no processes in inner nodes": if a unit has a main daemon process running in the main cgroup, and a reload control process would be started in the /control subcgroup, then this would necessarily fail, as the main daemon process would become an inner node process that way. We could in theory continue to support this in cgroupv1, but in the interest in keeping behaviour similar in both hierarchies, let's drop this altogether. Philosophically maybe it wasn't the greatest idea anyway to just go berserk and SIGKILL all those processes — loud warning logging might have sufficed, too. --- src/core/service.c | 55 +++++++++------------------------------------- 1 file changed, 10 insertions(+), 45 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 63a7172e65..9a50153fb5 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1343,11 +1343,6 @@ static int service_spawn( if (!final_env) return -ENOMEM; - if ((flags & EXEC_IS_CONTROL) && UNIT(s)->cgroup_path) { - exec_params.cgroup_path = strjoina(UNIT(s)->cgroup_path, "/control"); - (void) cg_create(SYSTEMD_CGROUP_CONTROLLER, exec_params.cgroup_path); - } - /* System services should get a new keyring by default. */ SET_FLAG(exec_params.flags, EXEC_NEW_KEYRING, MANAGER_IS_SYSTEM(UNIT(s)->manager)); @@ -1789,39 +1784,22 @@ fail: service_enter_stop(s, SERVICE_FAILURE_RESOURCES); } -static void service_kill_control_processes(Service *s) { +static void service_kill_control_process(Service *s) { int r; assert(s); - if (s->control_pid > 0) { - r = kill_and_sigcont(s->control_pid, SIGKILL); - if (r < 0) { - _cleanup_free_ char *comm = NULL; + if (s->control_pid <= 0) + return; - (void) get_process_comm(s->control_pid, &comm); + r = kill_and_sigcont(s->control_pid, SIGKILL); + if (r < 0) { + _cleanup_free_ char *comm = NULL; - log_unit_debug_errno(UNIT(s), r, "Failed to kill control process " PID_FMT " (%s), ignoring: %m", - s->control_pid, strna(comm)); - } - } + (void) get_process_comm(s->control_pid, &comm); - if (UNIT(s)->cgroup_path) { - _cleanup_set_free_ Set *pid_set = NULL; - char *p; - - if (s->control_pid > 0) { - r = set_make(&pid_set, PID_TO_PTR(s->control_pid), NULL); - if (r < 0) { - log_oom(); - return; - } - } - - p = strjoina(UNIT(s)->cgroup_path, "/control"); - r = cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, p, SIGKILL, CGROUP_SIGCONT|CGROUP_IGNORE_SELF|CGROUP_REMOVE, pid_set, NULL, NULL); - if (r < 0) - log_unit_debug_errno(UNIT(s), r, "Failed to send SIGKILL to processes of control group %s: %m", p); + log_unit_debug_errno(UNIT(s), r, "Failed to kill control process " PID_FMT " (%s), ignoring: %m", + s->control_pid, strna(comm)); } } @@ -1836,11 +1814,6 @@ static void service_enter_start(Service *s) { service_unwatch_control_pid(s); service_unwatch_main_pid(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. */ - service_kill_control_processes(s); - if (s->type == SERVICE_FORKING) { s->control_command_id = SERVICE_EXEC_START; c = s->control_command = s->exec_command[SERVICE_EXEC_START]; @@ -1927,9 +1900,6 @@ static void service_enter_start_pre(Service *s) { s->control_command = s->exec_command[SERVICE_EXEC_START_PRE]; if (s->control_command) { - /* Before we start anything, let's clear up what might - * be left from previous runs. */ - service_kill_control_processes(s); s->control_command_id = SERVICE_EXEC_START_PRE; @@ -3084,11 +3054,6 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { if (s->result == SERVICE_SUCCESS) s->result = f; - /* Immediately get rid of the cgroup, so that the - * kernel doesn't delay the cgroup empty messages for - * the service cgroup any longer than necessary */ - service_kill_control_processes(s); - if (s->control_command && s->control_command->command_next && f == SERVICE_SUCCESS) { @@ -3251,7 +3216,7 @@ static int service_dispatch_timer(sd_event_source *source, usec_t usec, void *us case SERVICE_RELOAD: log_unit_warning(UNIT(s), "Reload operation timed out. Killing reload process."); - service_kill_control_processes(s); + service_kill_control_process(s); s->reload_result = SERVICE_FAILURE_TIMEOUT; service_enter_running(s, SERVICE_SUCCESS); break; From e98b2fbbe9b48669f04e0e01eadf668be388fef4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 Nov 2017 20:27:01 +0100 Subject: [PATCH 7/9] core: generalize the cgroup empty check on GC Let's move the cgroup empty check for all unit types into the generic unit_check_gc() call, out of the per-unit-type _check_gc() type. This not only allows us to share some code, but also hooks up mount and socket units with this kind of check, for free, as it was missing there previously. --- src/core/scope.c | 14 -------------- src/core/service.c | 9 +++++---- src/core/unit.c | 12 ++++++++++++ 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/src/core/scope.c b/src/core/scope.c index e0e25673cb..10454d56b0 100644 --- a/src/core/scope.c +++ b/src/core/scope.c @@ -460,18 +460,6 @@ static int scope_deserialize_item(Unit *u, const char *key, const char *value, F return 0; } -static bool scope_check_gc(Unit *u) { - assert(u); - - /* Never clean up scopes that still have a process around, - * even if the scope is formally dead. */ - - if (!u->cgroup_path) - return false; - - return cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path) <= 0; -} - static void scope_notify_cgroup_empty_event(Unit *u) { Scope *s = SCOPE(u); assert(u); @@ -639,8 +627,6 @@ const UnitVTable scope_vtable = { .active_state = scope_active_state, .sub_state_to_string = scope_sub_state_to_string, - .check_gc = scope_check_gc, - .sigchld_event = scope_sigchld_event, .reset_failed = scope_reset_failed, diff --git a/src/core/service.c b/src/core/service.c index 9a50153fb5..efd808784d 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2716,10 +2716,11 @@ static bool service_check_gc(Unit *u) { assert(s); - /* Never clean up services that still have a process around, - * even if the service is formally dead. */ - if (cgroup_good(s) > 0 || - main_pid_good(s) > 0 || + /* Never clean up services that still have a process around, even if the service is formally dead. Note that + * unit_check_gc() already checked our cgroup for us, we just check our two additional PIDs, too, in case they + * have moved outside of the cgroup. */ + + if (main_pid_good(s) > 0 || control_pid_good(s) > 0) return true; diff --git a/src/core/unit.c b/src/core/unit.c index a5feb11ddb..2ddd95c3ac 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -334,6 +334,7 @@ int unit_set_description(Unit *u, const char *description) { bool unit_check_gc(Unit *u) { UnitActiveState state; + int r; assert(u); @@ -381,6 +382,17 @@ bool unit_check_gc(Unit *u) { assert_not_reached("Unknown garbage collection mode"); } + if (u->cgroup_path) { + /* If the unit has a cgroup, then check whether there's anything in it. If so, we should stay + * around. Units with active processes should never be collected. */ + + r = cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path); + if (r < 0) + log_unit_debug_errno(u, r, "Failed to determine whether cgroup %s is empty: %m", u->cgroup_path); + if (r <= 0) + return true; + } + if (UNIT_VTABLE(u)->check_gc) if (UNIT_VTABLE(u)->check_gc(u)) return true; From a4634b214c59631763b2c8dbd012eb3dca8ac91a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 Nov 2017 22:02:22 +0100 Subject: [PATCH 8/9] 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 4208d1de81..78ef885b06 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 00df3bb368..0c5bb4a2c8 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 1500670b09..b25bb9cb40 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 efd808784d..672eaa9199 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 572a70a2a8..7e3630ada7 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 115e59c5ca..849ccbdd43 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 2ddd95c3ac..856939853a 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 e33597c4b7..cd7c08a20d 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, ...) \ From 48f1b5e51d855c6a8c24f5e0cb6259327291a8b2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 Nov 2017 18:29:29 +0100 Subject: [PATCH 9/9] update TODO --- TODO | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/TODO b/TODO index 9bd68a0ab1..424a75df1a 100644 --- a/TODO +++ b/TODO @@ -24,6 +24,12 @@ Janitorial Clean-ups: Features: +* add some special mode to LogsDirectory=/StateDirectory=… that allows + declaring these directories without necessarily pulling in deps for them, or + creating them when starting up. That way, we could declare that + systemd-journald writes to /var/log/journal, which could be useful when we + doing disk usage calculations and so on. + * add a new RuntimeDirectoryPreserve= mode that defines a similar lifecycle for the runtime dir as we maintain for the fdstore: i.e. keep it around as long as the unit is running or has a job queued. @@ -45,8 +51,6 @@ Features: * add a way to lock down cgroup migration: a boolean, which when set for a unit makes sure the processes in it can never migrate out of it -* complain if a unit starts up and there are already processes in its cgroup - * blog about fd store and restartable services * document Environment=SYSTEMD_LOG_LEVEL=debug drop-in in debugging document @@ -303,8 +307,6 @@ Features: the specified range and generates sane error messages for incorrect specifications. -* do something about "/control" subcgroups in the unified cgroup hierarchy - * when we detect that there are waiting jobs but no running jobs, do something * push CPUAffinity= also into the "cpuset" cgroup controller (only after the cpuset controller got ported to the unified hierarchy)