From 3386f66200d53099674bed434eb5942b2ad0067a Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 30 Jul 2024 15:40:25 +0200 Subject: [PATCH 01/13] cgroup-setup: drop unused cg_migrate_callback for cg_attach_everywhere() While at it, move the typedef from cgroup-util to -setup. --- src/basic/cgroup-util.h | 2 -- src/core/exec-invoke.c | 2 +- src/core/unit.c | 2 +- src/shared/cgroup-setup.c | 13 +++++-------- src/shared/cgroup-setup.h | 14 +++++++++----- src/shared/tests.c | 2 +- src/test/test-bpf-devices.c | 2 +- 7 files changed, 18 insertions(+), 19 deletions(-) diff --git a/src/basic/cgroup-util.h b/src/basic/cgroup-util.h index 84009b47c1..28c11ef3e2 100644 --- a/src/basic/cgroup-util.h +++ b/src/basic/cgroup-util.h @@ -304,8 +304,6 @@ bool cg_controller_is_valid(const char *p); int cg_slice_to_path(const char *unit, char **ret); -typedef const char* (*cg_migrate_callback_t)(CGroupMask mask, void *userdata); - int cg_mask_supported(CGroupMask *ret); int cg_mask_supported_subtree(const char *root, CGroupMask *ret); int cg_mask_from_string(const char *s, CGroupMask *ret); diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index 71b5a58676..cf17824fe8 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -4351,7 +4351,7 @@ int exec_invoke( return log_exec_error_errno(context, params, r, "Failed to acquire cgroup path: %m"); } - r = cg_attach_everywhere(params->cgroup_supported, p, 0, NULL, NULL); + r = cg_attach_everywhere(params->cgroup_supported, p, 0); if (r == -EUCLEAN) { *exit_status = EXIT_CGROUP; return log_exec_error_errno(context, params, r, diff --git a/src/core/unit.c b/src/core/unit.c index be29a4d9ef..b205585054 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -5424,7 +5424,7 @@ int unit_fork_helper_process(Unit *u, const char *name, PidRef *ret) { (void) ignore_signals(SIGPIPE); if (crt->cgroup_path) { - r = cg_attach_everywhere(u->manager->cgroup_supported, crt->cgroup_path, 0, NULL, NULL); + r = cg_attach_everywhere(u->manager->cgroup_supported, crt->cgroup_path, 0); if (r < 0) { log_unit_error_errno(u, r, "Failed to join unit cgroup %s: %m", empty_to_root(crt->cgroup_path)); _exit(EXIT_CGROUP); diff --git a/src/shared/cgroup-setup.c b/src/shared/cgroup-setup.c index 093b6d0d22..63ac4bc3a7 100644 --- a/src/shared/cgroup-setup.c +++ b/src/shared/cgroup-setup.c @@ -788,9 +788,12 @@ int cg_create_everywhere(CGroupMask supported, CGroupMask mask, const char *path return created; } -int cg_attach_everywhere(CGroupMask supported, const char *path, pid_t pid, cg_migrate_callback_t path_callback, void *userdata) { +int cg_attach_everywhere(CGroupMask supported, const char *path, pid_t pid) { int r; + assert(path); + assert(pid >= 0); + r = cg_attach(SYSTEMD_CGROUP_CONTROLLER, path, pid); if (r < 0) return r; @@ -806,7 +809,6 @@ int cg_attach_everywhere(CGroupMask supported, const char *path, pid_t pid, cg_m for (CGroupController c = 0; c < _CGROUP_CONTROLLER_MAX; c++) { CGroupMask bit = CGROUP_CONTROLLER_TO_MASK(c); - const char *p = NULL; if (!FLAGS_SET(supported, bit)) continue; @@ -814,12 +816,7 @@ int cg_attach_everywhere(CGroupMask supported, const char *path, pid_t pid, cg_m if (FLAGS_SET(done, bit)) continue; - if (path_callback) - p = path_callback(bit, userdata); - if (!p) - p = path; - - (void) cg_attach_fallback(cgroup_controller_to_string(c), p, pid); + (void) cg_attach_fallback(cgroup_controller_to_string(c), path, pid); done |= CGROUP_MASK_EXTEND_JOINED(bit); } diff --git a/src/shared/cgroup-setup.h b/src/shared/cgroup-setup.h index 283ab6757c..d5ded0eaae 100644 --- a/src/shared/cgroup-setup.h +++ b/src/shared/cgroup-setup.h @@ -28,13 +28,17 @@ int cg_create_and_attach(const char *controller, const char *path, pid_t pid); int cg_set_access(const char *controller, const char *path, uid_t uid, gid_t gid); int cg_set_access_recursive(const char *controller, const char *path, uid_t uid, gid_t gid); +int cg_create_everywhere(CGroupMask supported, CGroupMask mask, const char *path); +int cg_attach_everywhere(CGroupMask supported, const char *path, pid_t pid); +int cg_trim_everywhere(CGroupMask supported, const char *path, bool delete_root); +int cg_enable_everywhere(CGroupMask supported, CGroupMask mask, const char *p, CGroupMask *ret_result_mask); + int cg_migrate(const char *cfrom, const char *pfrom, const char *cto, const char *pto, CGroupFlags flags); + +typedef const char* (*cg_migrate_callback_t)(CGroupMask mask, void *userdata); + +/* CGroup V1 specific */ int cg_migrate_recursive(const char *cfrom, const char *pfrom, const char *cto, const char *pto, CGroupFlags flags); int cg_migrate_recursive_fallback(const char *cfrom, const char *pfrom, const char *cto, const char *pto, CGroupFlags flags); - -int cg_create_everywhere(CGroupMask supported, CGroupMask mask, const char *path); -int cg_attach_everywhere(CGroupMask supported, const char *path, pid_t pid, cg_migrate_callback_t callback, void *userdata); int cg_migrate_v1_controllers(CGroupMask supported, CGroupMask mask, const char *from, cg_migrate_callback_t to_callback, void *userdata); -int cg_trim_everywhere(CGroupMask supported, const char *path, bool delete_root); int cg_trim_v1_controllers(CGroupMask supported, CGroupMask mask, const char *path, bool delete_root); -int cg_enable_everywhere(CGroupMask supported, CGroupMask mask, const char *p, CGroupMask *ret_result_mask); diff --git a/src/shared/tests.c b/src/shared/tests.c index e62795cb77..dbafed92c5 100644 --- a/src/shared/tests.c +++ b/src/shared/tests.c @@ -291,7 +291,7 @@ static int enter_cgroup(char **ret_cgroup, bool enter_subroot) { if (r < 0) return r; - r = cg_attach_everywhere(supported, cgroup_subroot, 0, NULL, NULL); + r = cg_attach_everywhere(supported, cgroup_subroot, 0); if (r < 0) return r; diff --git a/src/test/test-bpf-devices.c b/src/test/test-bpf-devices.c index f1db021c94..3993c35aa4 100644 --- a/src/test/test-bpf-devices.c +++ b/src/test/test-bpf-devices.c @@ -300,7 +300,7 @@ int main(int argc, char *argv[]) { ASSERT_OK(path_extract_directory(cgroup, &parent)); ASSERT_OK(cg_mask_supported(&supported)); - r = cg_attach_everywhere(supported, parent, 0, NULL, NULL); + r = cg_attach_everywhere(supported, parent, 0); ASSERT_OK(r); return 0; From d1397fb50c31c5058e6af03bac7f6cca519378ad Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 30 Jul 2024 15:52:02 +0200 Subject: [PATCH 02/13] cgroup-setup: group v1-specific functions --- src/shared/cgroup-setup.c | 296 +++++++++++++++++++------------------- 1 file changed, 148 insertions(+), 148 deletions(-) diff --git a/src/shared/cgroup-setup.c b/src/shared/cgroup-setup.c index 63ac4bc3a7..c89c2c1d40 100644 --- a/src/shared/cgroup-setup.c +++ b/src/shared/cgroup-setup.c @@ -653,89 +653,6 @@ int cg_migrate( return ret; } -int cg_migrate_recursive( - const char *cfrom, - const char *pfrom, - const char *cto, - const char *pto, - CGroupFlags flags) { - - _cleanup_closedir_ DIR *d = NULL; - int r, ret = 0; - char *fn; - - assert(cfrom); - assert(pfrom); - assert(cto); - assert(pto); - - ret = cg_migrate(cfrom, pfrom, cto, pto, flags); - - r = cg_enumerate_subgroups(cfrom, pfrom, &d); - if (r < 0) { - if (ret >= 0 && r != -ENOENT) - return r; - - return ret; - } - - while ((r = cg_read_subgroup(d, &fn)) > 0) { - _cleanup_free_ char *p = NULL; - - p = path_join(empty_to_root(pfrom), fn); - free(fn); - if (!p) - return -ENOMEM; - - r = cg_migrate_recursive(cfrom, p, cto, pto, flags); - if (r != 0 && ret >= 0) - ret = r; - } - - if (r < 0 && ret >= 0) - ret = r; - - if (flags & CGROUP_REMOVE) { - r = cg_rmdir(cfrom, pfrom); - if (r < 0 && ret >= 0 && !IN_SET(r, -ENOENT, -EBUSY)) - return r; - } - - return ret; -} - -int cg_migrate_recursive_fallback( - const char *cfrom, - const char *pfrom, - const char *cto, - const char *pto, - CGroupFlags flags) { - - int r; - - assert(cfrom); - assert(pfrom); - assert(cto); - assert(pto); - - r = cg_migrate_recursive(cfrom, pfrom, cto, pto, flags); - if (r < 0) { - char prefix[strlen(pto) + 1]; - - /* This didn't work? Then let's try all prefixes of the destination */ - - PATH_FOREACH_PREFIX(prefix, pto) { - int q; - - q = cg_migrate_recursive(cfrom, pfrom, cto, prefix, flags); - if (q >= 0) - return q; - } - } - - return r; -} - int cg_create_everywhere(CGroupMask supported, CGroupMask mask, const char *path) { CGroupController c; CGroupMask done; @@ -823,42 +740,6 @@ int cg_attach_everywhere(CGroupMask supported, const char *path, pid_t pid) { return 0; } -int cg_migrate_v1_controllers(CGroupMask supported, CGroupMask mask, const char *from, cg_migrate_callback_t to_callback, void *userdata) { - CGroupController c; - CGroupMask done; - int r = 0, q; - - assert(to_callback); - - supported &= CGROUP_MASK_V1; - mask = CGROUP_MASK_EXTEND_JOINED(mask); - done = 0; - - for (c = 0; c < _CGROUP_CONTROLLER_MAX; c++) { - CGroupMask bit = CGROUP_CONTROLLER_TO_MASK(c); - const char *to = NULL; - - if (!FLAGS_SET(supported, bit)) - continue; - - if (FLAGS_SET(done, bit)) - continue; - - if (!FLAGS_SET(mask, bit)) - continue; - - to = to_callback(bit, userdata); - - /* Remember first error and try continuing */ - q = cg_migrate_recursive_fallback(SYSTEMD_CGROUP_CONTROLLER, from, cgroup_controller_to_string(c), to, 0); - r = (r < 0) ? r : q; - - done |= CGROUP_MASK_EXTEND_JOINED(bit); - } - - return r; -} - int cg_trim_everywhere(CGroupMask supported, const char *path, bool delete_root) { int r, q; @@ -875,35 +756,6 @@ int cg_trim_everywhere(CGroupMask supported, const char *path, bool delete_root) return cg_trim_v1_controllers(supported, _CGROUP_MASK_ALL, path, delete_root); } -int cg_trim_v1_controllers(CGroupMask supported, CGroupMask mask, const char *path, bool delete_root) { - CGroupController c; - CGroupMask done; - int r = 0, q; - - supported &= CGROUP_MASK_V1; - mask = CGROUP_MASK_EXTEND_JOINED(mask); - done = 0; - - for (c = 0; c < _CGROUP_CONTROLLER_MAX; c++) { - CGroupMask bit = CGROUP_CONTROLLER_TO_MASK(c); - - if (!FLAGS_SET(supported, bit)) - continue; - - if (FLAGS_SET(done, bit)) - continue; - - if (FLAGS_SET(mask, bit)) { - /* Remember first error and try continuing */ - q = cg_trim(cgroup_controller_to_string(c), path, delete_root); - r = (r < 0) ? r : q; - } - done |= CGROUP_MASK_EXTEND_JOINED(bit); - } - - return r; -} - int cg_enable_everywhere( CGroupMask supported, CGroupMask mask, @@ -1011,3 +863,151 @@ int cg_enable_everywhere( return 0; } + +int cg_migrate_recursive( + const char *cfrom, + const char *pfrom, + const char *cto, + const char *pto, + CGroupFlags flags) { + + _cleanup_closedir_ DIR *d = NULL; + int r, ret = 0; + char *fn; + + assert(cfrom); + assert(pfrom); + assert(cto); + assert(pto); + + ret = cg_migrate(cfrom, pfrom, cto, pto, flags); + + r = cg_enumerate_subgroups(cfrom, pfrom, &d); + if (r < 0) { + if (ret >= 0 && r != -ENOENT) + return r; + + return ret; + } + + while ((r = cg_read_subgroup(d, &fn)) > 0) { + _cleanup_free_ char *p = NULL; + + p = path_join(empty_to_root(pfrom), fn); + free(fn); + if (!p) + return -ENOMEM; + + r = cg_migrate_recursive(cfrom, p, cto, pto, flags); + if (r != 0 && ret >= 0) + ret = r; + } + + if (r < 0 && ret >= 0) + ret = r; + + if (flags & CGROUP_REMOVE) { + r = cg_rmdir(cfrom, pfrom); + if (r < 0 && ret >= 0 && !IN_SET(r, -ENOENT, -EBUSY)) + return r; + } + + return ret; +} + +int cg_migrate_recursive_fallback( + const char *cfrom, + const char *pfrom, + const char *cto, + const char *pto, + CGroupFlags flags) { + + int r; + + assert(cfrom); + assert(pfrom); + assert(cto); + assert(pto); + + r = cg_migrate_recursive(cfrom, pfrom, cto, pto, flags); + if (r < 0) { + char prefix[strlen(pto) + 1]; + + /* This didn't work? Then let's try all prefixes of the destination */ + + PATH_FOREACH_PREFIX(prefix, pto) { + int q; + + q = cg_migrate_recursive(cfrom, pfrom, cto, prefix, flags); + if (q >= 0) + return q; + } + } + + return r; +} + +int cg_migrate_v1_controllers(CGroupMask supported, CGroupMask mask, const char *from, cg_migrate_callback_t to_callback, void *userdata) { + CGroupController c; + CGroupMask done; + int r = 0, q; + + assert(to_callback); + + supported &= CGROUP_MASK_V1; + mask = CGROUP_MASK_EXTEND_JOINED(mask); + done = 0; + + for (c = 0; c < _CGROUP_CONTROLLER_MAX; c++) { + CGroupMask bit = CGROUP_CONTROLLER_TO_MASK(c); + const char *to = NULL; + + if (!FLAGS_SET(supported, bit)) + continue; + + if (FLAGS_SET(done, bit)) + continue; + + if (!FLAGS_SET(mask, bit)) + continue; + + to = to_callback(bit, userdata); + + /* Remember first error and try continuing */ + q = cg_migrate_recursive_fallback(SYSTEMD_CGROUP_CONTROLLER, from, cgroup_controller_to_string(c), to, 0); + r = (r < 0) ? r : q; + + done |= CGROUP_MASK_EXTEND_JOINED(bit); + } + + return r; +} + +int cg_trim_v1_controllers(CGroupMask supported, CGroupMask mask, const char *path, bool delete_root) { + CGroupController c; + CGroupMask done; + int r = 0, q; + + supported &= CGROUP_MASK_V1; + mask = CGROUP_MASK_EXTEND_JOINED(mask); + done = 0; + + for (c = 0; c < _CGROUP_CONTROLLER_MAX; c++) { + CGroupMask bit = CGROUP_CONTROLLER_TO_MASK(c); + + if (!FLAGS_SET(supported, bit)) + continue; + + if (FLAGS_SET(done, bit)) + continue; + + if (FLAGS_SET(mask, bit)) { + /* Remember first error and try continuing */ + q = cg_trim(cgroup_controller_to_string(c), path, delete_root); + r = (r < 0) ? r : q; + } + done |= CGROUP_MASK_EXTEND_JOINED(bit); + } + + return r; +} From ea25672de5a22241b068050f812d61501de5f1a9 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 30 Jul 2024 16:15:59 +0200 Subject: [PATCH 03/13] cgroup-setup: move cg_{,un}install_release_agent from cgroup-util They're pid1-specific, so move them out of basic/. --- src/basic/cgroup-util.c | 85 --------------------------------------- src/basic/cgroup-util.h | 3 -- src/core/main.c | 1 + src/shared/cgroup-setup.c | 85 +++++++++++++++++++++++++++++++++++++++ src/shared/cgroup-setup.h | 3 ++ 5 files changed, 89 insertions(+), 88 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index ffce7504f4..5a2636841e 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -870,91 +870,6 @@ int cg_pidref_get_path(const char *controller, const PidRef *pidref, char **ret_ return 0; } -int cg_install_release_agent(const char *controller, const char *agent) { - _cleanup_free_ char *fs = NULL, *contents = NULL; - const char *sc; - int r; - - assert(agent); - - r = cg_unified_controller(controller); - if (r < 0) - return r; - if (r > 0) /* doesn't apply to unified hierarchy */ - return -EOPNOTSUPP; - - r = cg_get_path(controller, NULL, "release_agent", &fs); - if (r < 0) - return r; - - r = read_one_line_file(fs, &contents); - if (r < 0) - return r; - - sc = strstrip(contents); - if (isempty(sc)) { - r = write_string_file(fs, agent, WRITE_STRING_FILE_DISABLE_BUFFER); - if (r < 0) - return r; - } else if (!path_equal(sc, agent)) - return -EEXIST; - - fs = mfree(fs); - r = cg_get_path(controller, NULL, "notify_on_release", &fs); - if (r < 0) - return r; - - contents = mfree(contents); - r = read_one_line_file(fs, &contents); - if (r < 0) - return r; - - sc = strstrip(contents); - if (streq(sc, "0")) { - r = write_string_file(fs, "1", WRITE_STRING_FILE_DISABLE_BUFFER); - if (r < 0) - return r; - - return 1; - } - - if (!streq(sc, "1")) - return -EIO; - - return 0; -} - -int cg_uninstall_release_agent(const char *controller) { - _cleanup_free_ char *fs = NULL; - int r; - - r = cg_unified_controller(controller); - if (r < 0) - return r; - if (r > 0) /* Doesn't apply to unified hierarchy */ - return -EOPNOTSUPP; - - r = cg_get_path(controller, NULL, "notify_on_release", &fs); - if (r < 0) - return r; - - r = write_string_file(fs, "0", WRITE_STRING_FILE_DISABLE_BUFFER); - if (r < 0) - return r; - - fs = mfree(fs); - - r = cg_get_path(controller, NULL, "release_agent", &fs); - if (r < 0) - return r; - - r = write_string_file(fs, "", WRITE_STRING_FILE_DISABLE_BUFFER); - if (r < 0) - return r; - - return 0; -} - int cg_is_empty(const char *controller, const char *path) { _cleanup_fclose_ FILE *f = NULL; pid_t pid; diff --git a/src/basic/cgroup-util.h b/src/basic/cgroup-util.h index 28c11ef3e2..5449d38894 100644 --- a/src/basic/cgroup-util.h +++ b/src/basic/cgroup-util.h @@ -263,9 +263,6 @@ int cg_get_xattr_malloc(const char *path, const char *name, char **ret); int cg_get_xattr_bool(const char *path, const char *name); int cg_remove_xattr(const char *path, const char *name); -int cg_install_release_agent(const char *controller, const char *agent); -int cg_uninstall_release_agent(const char *controller); - int cg_is_empty(const char *controller, const char *path); int cg_is_empty_recursive(const char *controller, const char *path); diff --git a/src/core/main.c b/src/core/main.c index 078b968ae4..4c92d81c40 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -27,6 +27,7 @@ #include "bus-error.h" #include "bus-util.h" #include "capability-util.h" +#include "cgroup-setup.h" #include "cgroup-util.h" #include "chase.h" #include "clock-util.h" diff --git a/src/shared/cgroup-setup.c b/src/shared/cgroup-setup.c index c89c2c1d40..d7e27a1837 100644 --- a/src/shared/cgroup-setup.c +++ b/src/shared/cgroup-setup.c @@ -1011,3 +1011,88 @@ int cg_trim_v1_controllers(CGroupMask supported, CGroupMask mask, const char *pa return r; } + +int cg_install_release_agent(const char *controller, const char *agent) { + _cleanup_free_ char *fs = NULL, *contents = NULL; + const char *sc; + int r; + + assert(agent); + + r = cg_unified_controller(controller); + if (r < 0) + return r; + if (r > 0) /* doesn't apply to unified hierarchy */ + return -EOPNOTSUPP; + + r = cg_get_path(controller, NULL, "release_agent", &fs); + if (r < 0) + return r; + + r = read_one_line_file(fs, &contents); + if (r < 0) + return r; + + sc = strstrip(contents); + if (isempty(sc)) { + r = write_string_file(fs, agent, WRITE_STRING_FILE_DISABLE_BUFFER); + if (r < 0) + return r; + } else if (!path_equal(sc, agent)) + return -EEXIST; + + fs = mfree(fs); + r = cg_get_path(controller, NULL, "notify_on_release", &fs); + if (r < 0) + return r; + + contents = mfree(contents); + r = read_one_line_file(fs, &contents); + if (r < 0) + return r; + + sc = strstrip(contents); + if (streq(sc, "0")) { + r = write_string_file(fs, "1", WRITE_STRING_FILE_DISABLE_BUFFER); + if (r < 0) + return r; + + return 1; + } + + if (!streq(sc, "1")) + return -EIO; + + return 0; +} + +int cg_uninstall_release_agent(const char *controller) { + _cleanup_free_ char *fs = NULL; + int r; + + r = cg_unified_controller(controller); + if (r < 0) + return r; + if (r > 0) /* Doesn't apply to unified hierarchy */ + return -EOPNOTSUPP; + + r = cg_get_path(controller, NULL, "notify_on_release", &fs); + if (r < 0) + return r; + + r = write_string_file(fs, "0", WRITE_STRING_FILE_DISABLE_BUFFER); + if (r < 0) + return r; + + fs = mfree(fs); + + r = cg_get_path(controller, NULL, "release_agent", &fs); + if (r < 0) + return r; + + r = write_string_file(fs, "", WRITE_STRING_FILE_DISABLE_BUFFER); + if (r < 0) + return r; + + return 0; +} diff --git a/src/shared/cgroup-setup.h b/src/shared/cgroup-setup.h index d5ded0eaae..b8473097cf 100644 --- a/src/shared/cgroup-setup.h +++ b/src/shared/cgroup-setup.h @@ -42,3 +42,6 @@ int cg_migrate_recursive(const char *cfrom, const char *pfrom, const char *cto, int cg_migrate_recursive_fallback(const char *cfrom, const char *pfrom, const char *cto, const char *pto, CGroupFlags flags); int cg_migrate_v1_controllers(CGroupMask supported, CGroupMask mask, const char *from, cg_migrate_callback_t to_callback, void *userdata); int cg_trim_v1_controllers(CGroupMask supported, CGroupMask mask, const char *path, bool delete_root); + +int cg_install_release_agent(const char *controller, const char *agent); +int cg_uninstall_release_agent(const char *controller); From 279c5a343f23645b771993c8a4fec8a6f73eaf59 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 30 Jul 2024 16:10:06 +0200 Subject: [PATCH 04/13] cgroup-setup: minor cleanups --- src/shared/cgroup-setup.c | 85 +++++++++++++++++++++++---------------- 1 file changed, 51 insertions(+), 34 deletions(-) diff --git a/src/shared/cgroup-setup.c b/src/shared/cgroup-setup.c index d7e27a1837..016da01eb3 100644 --- a/src/shared/cgroup-setup.c +++ b/src/shared/cgroup-setup.c @@ -171,6 +171,9 @@ int cg_weight_parse(const char *s, uint64_t *ret) { uint64_t u; int r; + assert(s); + assert(ret); + if (isempty(s)) { *ret = CGROUP_WEIGHT_INVALID; return 0; @@ -188,8 +191,12 @@ int cg_weight_parse(const char *s, uint64_t *ret) { } int cg_cpu_weight_parse(const char *s, uint64_t *ret) { - if (streq_ptr(s, "idle")) + assert(s); + assert(ret); + + if (streq(s, "idle")) return *ret = CGROUP_WEIGHT_IDLE; + return cg_weight_parse(s, ret); } @@ -197,6 +204,9 @@ int cg_cpu_shares_parse(const char *s, uint64_t *ret) { uint64_t u; int r; + assert(s); + assert(ret); + if (isempty(s)) { *ret = CGROUP_CPU_SHARES_INVALID; return 0; @@ -217,6 +227,9 @@ int cg_blkio_weight_parse(const char *s, uint64_t *ret) { uint64_t u; int r; + assert(s); + assert(ret); + if (isempty(s)) { *ret = CGROUP_BLKIO_WEIGHT_INVALID; return 0; @@ -256,7 +269,6 @@ int cg_trim(const char *controller, const char *path, bool delete_root) { _cleanup_free_ char *fs = NULL; int r, q; - assert(path); assert(controller); r = cg_get_path(controller, path, NULL, &fs); @@ -266,15 +278,15 @@ int cg_trim(const char *controller, const char *path, bool delete_root) { r = recurse_dir_at( AT_FDCWD, fs, - /* statx_mask= */ 0, - /* n_depth_max= */ UINT_MAX, + /* statx_mask = */ 0, + /* n_depth_max = */ UINT_MAX, RECURSE_DIR_ENSURE_TYPE, trim_cb, - NULL); + /* userdata = */ NULL); if (r == -ENOENT) /* non-existing is the ultimate trimming, hence no error */ r = 0; else if (r < 0) - log_debug_errno(r, "Failed to iterate through cgroup %s: %m", path); + log_debug_errno(r, "Failed to trim subcgroups of '%s': %m", path); /* If we shall delete the top-level cgroup, then propagate the failure to do so (except if it is * already gone anyway). Also, let's debug log about this failure, except if the error code is an @@ -282,9 +294,8 @@ int cg_trim(const char *controller, const char *path, bool delete_root) { if (delete_root && !empty_or_root(path) && rmdir(fs) < 0 && errno != ENOENT) { if (!IN_SET(errno, ENOTEMPTY, EBUSY)) - log_debug_errno(errno, "Failed to trim cgroup %s: %m", path); - if (r >= 0) - r = -errno; + log_debug_errno(errno, "Failed to trim cgroup '%s': %m", path); + RET_GATHER(r, -errno); } q = cg_hybrid_unified(); @@ -303,6 +314,8 @@ int cg_create(const char *controller, const char *path) { _cleanup_free_ char *fs = NULL; int r; + assert(controller); + r = cg_get_path_and_check(controller, path, NULL, &fs); if (r < 0) return r; @@ -320,38 +333,21 @@ int cg_create(const char *controller, const char *path) { r = cg_hybrid_unified(); if (r < 0) return r; - if (r > 0 && streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { r = cg_create(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path); if (r < 0) - log_warning_errno(r, "Failed to create compat systemd cgroup %s: %m", path); + log_warning_errno(r, "Failed to create compat systemd cgroup '%s', ignoring: %m", path); } return 1; } -int cg_create_and_attach(const char *controller, const char *path, pid_t pid) { - int r, q; - - assert(pid >= 0); - - r = cg_create(controller, path); - if (r < 0) - return r; - - q = cg_attach(controller, path, pid); - if (q < 0) - return q; - - /* This does not remove the cgroup on failure */ - return r; -} - int cg_attach(const char *controller, const char *path, pid_t pid) { _cleanup_free_ char *fs = NULL; char c[DECIMAL_STR_MAX(pid_t) + 2]; int r; + assert(controller); assert(path); assert(pid >= 0); @@ -374,11 +370,10 @@ int cg_attach(const char *controller, const char *path, pid_t pid) { r = cg_hybrid_unified(); if (r < 0) return r; - if (r > 0 && streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { r = cg_attach(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path, pid); if (r < 0) - log_warning_errno(r, "Failed to attach "PID_FMT" to compat systemd cgroup %s: %m", pid, path); + log_warning_errno(r, "Failed to attach "PID_FMT" to compat systemd cgroup '%s', ignoring: %m", pid, path); } return 0; @@ -409,8 +404,7 @@ int cg_attach_fallback(const char *controller, const char *path, pid_t pid) { if (r < 0) { char prefix[strlen(path) + 1]; - /* This didn't work? Then let's try all prefixes of - * the destination */ + /* This didn't work? Then let's try all prefixes of the destination */ PATH_FOREACH_PREFIX(prefix, path) { int q; @@ -424,6 +418,24 @@ int cg_attach_fallback(const char *controller, const char *path, pid_t pid) { return r; } +int cg_create_and_attach(const char *controller, const char *path, pid_t pid) { + int r, q; + + /* This does not remove the cgroup on failure */ + + assert(pid >= 0); + + r = cg_create(controller, path); + if (r < 0) + return r; + + q = cg_attach(controller, path, pid); + if (q < 0) + return q; + + return r; +} + int cg_set_access( const char *controller, const char *path, @@ -555,9 +567,12 @@ int cg_set_access_recursive( _cleanup_free_ char *fs = NULL; int r; + assert(controller); + assert(path); + /* A recursive version of cg_set_access(). But note that this one changes ownership of *all* files, * not just the allowlist that cg_set_access() uses. Use cg_set_access() on the cgroup you want to - * delegate, and cg_set_access_recursive() for any subcrgoups you might want to create below it. */ + * delegate, and cg_set_access_recursive() for any subcgroups you might want to create below it. */ if (!uid_is_valid(uid) && !gid_is_valid(gid)) return 0; @@ -595,8 +610,8 @@ int cg_migrate( const char *pto, CGroupFlags flags) { - bool done = false; _cleanup_set_free_ Set *s = NULL; + bool done; int r, ret = 0; assert(cfrom); @@ -743,6 +758,8 @@ int cg_attach_everywhere(CGroupMask supported, const char *path, pid_t pid) { int cg_trim_everywhere(CGroupMask supported, const char *path, bool delete_root) { int r, q; + assert(path); + r = cg_trim(SYSTEMD_CGROUP_CONTROLLER, path, delete_root); if (r < 0) return r; From 8b7191aee4f4edf00c3e2a9c112b70ae7b63fedf Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 30 Jul 2024 17:42:33 +0200 Subject: [PATCH 05/13] cgroup-setup: use fchownat() + AT_EMPTY_PATH where appropriate This already existed long before our kernel baseline. While at it, switch to RET_GATHER(). --- src/shared/cgroup-setup.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/shared/cgroup-setup.c b/src/shared/cgroup-setup.c index 016da01eb3..b1cb47900c 100644 --- a/src/shared/cgroup-setup.c +++ b/src/shared/cgroup-setup.c @@ -539,20 +539,16 @@ static int access_callback( const struct statx *sx, void *userdata) { - struct access_callback_data *d = ASSERT_PTR(userdata); - if (!IN_SET(event, RECURSE_DIR_ENTER, RECURSE_DIR_ENTRY)) return RECURSE_DIR_CONTINUE; + struct access_callback_data *d = ASSERT_PTR(userdata); + + assert(path); assert(inode_fd >= 0); - /* fchown() doesn't support O_PATH fds, hence we use the /proc/self/fd/ trick */ - if (chown(FORMAT_PROC_FD_PATH(inode_fd), d->uid, d->gid) < 0) { - log_debug_errno(errno, "Failed to change ownership of '%s', ignoring: %m", ASSERT_PTR(path)); - - if (d->error == 0) /* Return last error to caller */ - d->error = errno; - } + if (fchownat(inode_fd, "", d->uid, d->gid, AT_EMPTY_PATH) < 0) + RET_GATHER(d->error, log_debug_errno(errno, "Failed to change ownership of '%s', ignoring: %m", path)); return RECURSE_DIR_CONTINUE; } @@ -600,7 +596,8 @@ int cg_set_access_recursive( if (r < 0) return r; - return -d.error; + assert(d.error <= 0); + return d.error; } int cg_migrate( From e563c4213e1705c16e682424a64f6445419321f6 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 30 Jul 2024 17:45:19 +0200 Subject: [PATCH 06/13] cgroup-setup: drop unneeded O_RDONLY when O_DIRECTORY --- src/shared/cgroup-setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/cgroup-setup.c b/src/shared/cgroup-setup.c index b1cb47900c..6292777369 100644 --- a/src/shared/cgroup-setup.c +++ b/src/shared/cgroup-setup.c @@ -577,7 +577,7 @@ int cg_set_access_recursive( if (r < 0) return r; - fd = open(fs, O_DIRECTORY|O_CLOEXEC|O_RDONLY); + fd = open(fs, O_DIRECTORY|O_CLOEXEC); if (fd < 0) return -errno; From 6e6d305573458c640b97046667bb39a0d0e6bfa8 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 30 Jul 2024 20:35:10 +0200 Subject: [PATCH 07/13] cgroup-setup: kernel threads can reside in arbitrary cgroups Realistically this doesn't matter, as cg_migrate() is only used to populate init.scope. But it's probably better to make things clear. --- src/shared/cgroup-setup.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/shared/cgroup-setup.c b/src/shared/cgroup-setup.c index 6292777369..cf7a0eb272 100644 --- a/src/shared/cgroup-setup.c +++ b/src/shared/cgroup-setup.c @@ -639,10 +639,7 @@ int cg_migrate( if (set_contains(s, PID_TO_PTR(pid))) continue; - /* Ignore kernel threads. Since they can only exist in the root cgroup, we only - * check for them there. */ - if (cfrom && empty_or_root(pfrom) && - pid_is_kernel_thread(pid) > 0) + if (pid_is_kernel_thread(pid) > 0) continue; r = cg_attach(cto, pto, pid); From 1daf5759900f022efd203ad9c78e1c6400176a03 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 30 Jul 2024 21:18:47 +0200 Subject: [PATCH 08/13] cgroup-util: refactor cg_{ns,freezer,kill}_supported --- src/basic/cgroup-util.c | 42 ++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 5a2636841e..de618545d7 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -208,19 +208,16 @@ int cg_read_event( } bool cg_ns_supported(void) { - static thread_local int enabled = -1; + static thread_local int supported = -1; - if (enabled >= 0) - return enabled; + if (supported >= 0) + return supported; - if (access("/proc/self/ns/cgroup", F_OK) < 0) { - if (errno != ENOENT) - log_debug_errno(errno, "Failed to check whether /proc/self/ns/cgroup is available, assuming not: %m"); - enabled = false; - } else - enabled = true; - - return enabled; + if (access("/proc/self/ns/cgroup", F_OK) >= 0) + return (supported = true); + if (errno != ENOENT) + log_debug_errno(errno, "Failed to check whether /proc/self/ns/cgroup is available, assuming not: %m"); + return (supported = false); } bool cg_freezer_supported(void) { @@ -229,9 +226,14 @@ bool cg_freezer_supported(void) { if (supported >= 0) return supported; - supported = cg_all_unified() > 0 && access("/sys/fs/cgroup/init.scope/cgroup.freeze", F_OK) == 0; + if (cg_all_unified() <= 0) + return (supported = false); - return supported; + if (access("/sys/fs/cgroup/init.scope/cgroup.freeze", F_OK) >= 0) + return (supported = true); + if (errno != ENOENT) + log_debug_errno(errno, "Failed to check whether cgroup freezer is available, assuming not: %m"); + return (supported = false); } bool cg_kill_supported(void) { @@ -241,15 +243,13 @@ bool cg_kill_supported(void) { return supported; if (cg_all_unified() <= 0) - supported = false; - else if (access("/sys/fs/cgroup/init.scope/cgroup.kill", F_OK) < 0) { - if (errno != ENOENT) - log_debug_errno(errno, "Failed to check if cgroup.kill is available, assuming not: %m"); - supported = false; - } else - supported = true; + return (supported = false); - return supported; + if (access("/sys/fs/cgroup/init.scope/cgroup.kill", F_OK) >= 0) + return (supported = true); + if (errno != ENOENT) + log_debug_errno(errno, "Failed to check whether cgroup.kill is available, assuming not: %m"); + return (supported = false); } int cg_enumerate_subgroups(const char *controller, const char *path, DIR **ret) { From 031860d6cbe430fb080ec0986950b72c4718e59a Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 30 Jul 2024 22:29:00 +0200 Subject: [PATCH 09/13] cgroup-util: drop unused cg_rmdir() When removing a cgroup, we always want to eliminate subcgroups first, i.e. use cg_trim(). And cg_rmdir() (along with CGROUP_REMOVE flag) is simply unused. Kill it. --- src/basic/cgroup-util.c | 31 ------------------------------- src/basic/cgroup-util.h | 7 ++----- src/shared/cgroup-setup.c | 6 ------ src/test/test-cgroup.c | 7 +------ 4 files changed, 3 insertions(+), 48 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index de618545d7..01af942cab 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -291,31 +291,6 @@ int cg_read_subgroup(DIR *d, char **ret) { return 0; } -int cg_rmdir(const char *controller, const char *path) { - _cleanup_free_ char *p = NULL; - int r; - - r = cg_get_path(controller, path, NULL, &p); - if (r < 0) - return r; - - r = rmdir(p); - if (r < 0 && errno != ENOENT) - return -errno; - - r = cg_hybrid_unified(); - if (r <= 0) - return r; - - if (streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { - r = cg_rmdir(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path); - if (r < 0) - log_warning_errno(r, "Failed to remove compat systemd cgroup %s: %m", path); - } - - return 0; -} - static int cg_kill_items( const char *path, int sig, @@ -532,12 +507,6 @@ int cg_kill_recursive( } } - if (FLAGS_SET(flags, CGROUP_REMOVE)) { - r = cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, path); - if (!IN_SET(r, -ENOENT, -EBUSY)) - RET_GATHER(ret, log_debug_errno(r, "Failed to remove cgroup '%s': %m", path)); - } - return ret; } diff --git a/src/basic/cgroup-util.h b/src/basic/cgroup-util.h index 5449d38894..9d7573d944 100644 --- a/src/basic/cgroup-util.h +++ b/src/basic/cgroup-util.h @@ -186,9 +186,8 @@ int cg_cgroupid_open(int fsfd, uint64_t id); typedef enum CGroupFlags { CGROUP_SIGCONT = 1 << 0, CGROUP_IGNORE_SELF = 1 << 1, - CGROUP_REMOVE = 1 << 2, - CGROUP_DONT_SKIP_UNMAPPED = 1 << 3, - CGROUP_NO_PIDFD = 1 << 4, + CGROUP_DONT_SKIP_UNMAPPED = 1 << 2, + CGROUP_NO_PIDFD = 1 << 3, } CGroupFlags; int cg_enumerate_processes(const char *controller, const char *path, FILE **ret); @@ -214,8 +213,6 @@ int cg_get_path_and_check(const char *controller, const char *path, const char * int cg_pid_get_path(const char *controller, pid_t pid, char **ret); int cg_pidref_get_path(const char *controller, const PidRef *pidref, char **ret); -int cg_rmdir(const char *controller, const char *path); - int cg_is_threaded(const char *path); int cg_is_delegated(const char *path); diff --git a/src/shared/cgroup-setup.c b/src/shared/cgroup-setup.c index cf7a0eb272..d415d13d41 100644 --- a/src/shared/cgroup-setup.c +++ b/src/shared/cgroup-setup.c @@ -917,12 +917,6 @@ int cg_migrate_recursive( if (r < 0 && ret >= 0) ret = r; - if (flags & CGROUP_REMOVE) { - r = cg_rmdir(cfrom, pfrom); - if (r < 0 && ret >= 0 && !IN_SET(r, -ENOENT, -EBUSY)) - return r; - } - return ret; } diff --git a/src/test/test-cgroup.c b/src/test/test-cgroup.c index 040e9e9c12..461576c4cf 100644 --- a/src/test/test-cgroup.c +++ b/src/test/test-cgroup.c @@ -122,12 +122,7 @@ TEST(cg_create) { assert_se(cg_kill_recursive(test_a, 0, 0, NULL, NULL, NULL) > 0); assert_se(cg_kill_recursive(test_b, 0, 0, NULL, NULL, NULL) == 0); - (void) cg_trim(SYSTEMD_CGROUP_CONTROLLER, test_b, false); - - assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, test_b) == 0); - assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, test_a) < 0); - assert_se(cg_migrate_recursive(SYSTEMD_CGROUP_CONTROLLER, test_a, SYSTEMD_CGROUP_CONTROLLER, here, 0) > 0); - assert_se(cg_rmdir(SYSTEMD_CGROUP_CONTROLLER, test_a) == 0); + ASSERT_OK(cg_trim(SYSTEMD_CGROUP_CONTROLLER, test_b, true)); } TEST(id) { From 95e631da434493d5642348a15a1d620d962aa3e8 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 30 Jul 2024 21:37:36 +0200 Subject: [PATCH 10/13] core/unit: unexport cg_kill log funcs, rather take in bool It seems unnecessary to duplicate the func ptrs everywhere. --- src/core/mount.c | 4 ++-- src/core/service.c | 8 ++++---- src/core/socket.c | 4 ++-- src/core/swap.c | 5 +++-- src/core/unit.c | 15 ++++++++------- src/core/unit.h | 5 +---- 6 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index e040f151f4..61aa8bb590 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -919,7 +919,7 @@ static void mount_enter_dead(Mount *m, MountResult f, bool flush_result) { 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); + unit_warn_leftover_processes(UNIT(m), /* start = */ false); mount_set_state(m, m->result != MOUNT_SUCCESS ? MOUNT_FAILED : MOUNT_DEAD); @@ -1176,7 +1176,7 @@ static void mount_enter_mounting(Mount *m) { if (source_is_dir) unit_warn_if_dir_nonempty(UNIT(m), m->where); - unit_warn_leftover_processes(UNIT(m), unit_log_leftover_process_start); + unit_warn_leftover_processes(UNIT(m), /* start = */ true); 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 4403b209bd..2c6d83babe 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2035,7 +2035,7 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart) end_state = SERVICE_FAILED; restart_state = SERVICE_FAILED_BEFORE_AUTO_RESTART; } - unit_warn_leftover_processes(UNIT(s), unit_log_leftover_process_stop); + unit_warn_leftover_processes(UNIT(s), /* start = */ false); if (!allow_restart) log_unit_debug(UNIT(s), "Service restart not allowed."); @@ -2387,11 +2387,11 @@ static int service_adverse_to_leftover_processes(Service *s) { * 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), unit_log_leftover_process_start) > 0 && + if (unit_warn_leftover_processes(UNIT(s), /* start = */ true) > 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), - "Will not start SendSIGKILL=no service of type KillMode=control-group or mixed while processes exist"); + return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(EBUSY), + "Will not start SendSIGKILL=no service of type KillMode=control-group or mixed while processes exist"); return 0; } diff --git a/src/core/socket.c b/src/core/socket.c index 5042e4b078..da9aa9a4a7 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -2061,7 +2061,7 @@ 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); + unit_warn_leftover_processes(UNIT(s), /* start = */ false); socket_set_state(s, s->result != SOCKET_SUCCESS ? SOCKET_FAILED : SOCKET_DEAD); @@ -2262,7 +2262,7 @@ static void socket_enter_start_pre(Socket *s) { socket_unwatch_control_pid(s); - unit_warn_leftover_processes(UNIT(s), unit_log_leftover_process_start); + unit_warn_leftover_processes(UNIT(s), /* start = */ true); 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 e6f6f78dc9..3b76ae33a3 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -670,7 +670,8 @@ 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); + unit_warn_leftover_processes(UNIT(s), /* start = */ false); + swap_set_state(s, s->result != SWAP_SUCCESS ? SWAP_FAILED : SWAP_DEAD); s->exec_runtime = exec_runtime_destroy(s->exec_runtime); @@ -752,7 +753,7 @@ static void swap_enter_activating(Swap *s) { assert(s); - unit_warn_leftover_processes(UNIT(s), unit_log_leftover_process_start); + unit_warn_leftover_processes(UNIT(s), /* start = */ true); 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 b205585054..fa28f67f34 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -5840,7 +5840,8 @@ 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(const PidRef *pid, int sig, void *userdata) { +static int unit_log_leftover_process_start(const PidRef *pid, int sig, void *userdata) { + const Unit *u = ASSERT_PTR(userdata); _cleanup_free_ char *comm = NULL; assert(pidref_is_set(pid)); @@ -5852,7 +5853,7 @@ int unit_log_leftover_process_start(const PidRef *pid, int sig, void *userdata) /* During start we print a warning */ - log_unit_warning(userdata, + log_unit_warning(u, "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->pid, strna(comm)); @@ -5860,7 +5861,8 @@ int unit_log_leftover_process_start(const PidRef *pid, int sig, void *userdata) return 1; } -int unit_log_leftover_process_stop(const PidRef *pid, int sig, void *userdata) { +static int unit_log_leftover_process_stop(const PidRef *pid, int sig, void *userdata) { + const Unit *u = ASSERT_PTR(userdata); _cleanup_free_ char *comm = NULL; assert(pidref_is_set(pid)); @@ -5872,20 +5874,19 @@ int unit_log_leftover_process_stop(const PidRef *pid, int sig, void *userdata) { /* During stop we only print an informational message */ - log_unit_info(userdata, + log_unit_info(u, "Unit process " PID_FMT " (%s) remains running after unit stopped.", pid->pid, strna(comm)); return 1; } -int unit_warn_leftover_processes(Unit *u, cg_kill_log_func_t log_func) { +int unit_warn_leftover_processes(Unit *u, bool start) { assert(u); (void) unit_pick_cgroup_path(u); CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt || !crt->cgroup_path) return 0; @@ -5894,7 +5895,7 @@ int unit_warn_leftover_processes(Unit *u, cg_kill_log_func_t log_func) { /* sig= */ 0, /* flags= */ 0, /* set= */ NULL, - log_func, + start ? unit_log_leftover_process_start : unit_log_leftover_process_stop, u); } diff --git a/src/core/unit.h b/src/core/unit.h index 115948cd26..bf736a5870 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -990,10 +990,7 @@ void unit_unlink_state_files(Unit *u); int unit_prepare_exec(Unit *u); -int unit_log_leftover_process_start(const PidRef* pid, int sig, void *userdata); -int unit_log_leftover_process_stop(const PidRef* pid, int sig, void *userdata); - -int unit_warn_leftover_processes(Unit *u, cg_kill_log_func_t log_func); +int unit_warn_leftover_processes(Unit *u, bool start); bool unit_needs_console(Unit *u); From 5ffa9490d663a74e941459432b2be139e93f2ff5 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 30 Jul 2024 21:44:36 +0200 Subject: [PATCH 11/13] core/unit: modernize log_kill() too --- src/core/unit.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index fa28f67f34..c8c34fc0d9 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4683,21 +4683,24 @@ int unit_make_transient(Unit *u) { return 0; } +static bool ignore_leftover_process(const char *comm) { + return comm && comm[0] == '('; /* Most likely our own helper process (PAM?), ignore */ +} + static int log_kill(const PidRef *pid, int sig, void *userdata) { + const Unit *u = ASSERT_PTR(userdata); _cleanup_free_ char *comm = NULL; assert(pidref_is_set(pid)); (void) pidref_get_comm(pid, &comm); - /* Don't log about processes marked with brackets, under the assumption that these are temporary processes - only, like for example systemd's own PAM stub process. */ - if (comm && comm[0] == '(') + if (ignore_leftover_process(comm)) /* Although we didn't log anything, as this callback is used in unit_kill_context we must return 1 * here to let the manager know that a process was killed. */ return 1; - log_unit_notice(userdata, + log_unit_notice(u, "Killing process " PID_FMT " (%s) with signal SIG%s.", pid->pid, strna(comm), @@ -4712,6 +4715,7 @@ static int operation_to_signal( bool *ret_noteworthy) { assert(c); + assert(ret_noteworthy); switch (k) { @@ -5836,10 +5840,6 @@ int unit_prepare_exec(Unit *u) { return 0; } -static bool ignore_leftover_process(const char *comm) { - return comm && comm[0] == '('; /* Most likely our own helper process (PAM?), ignore */ -} - static int unit_log_leftover_process_start(const PidRef *pid, int sig, void *userdata) { const Unit *u = ASSERT_PTR(userdata); _cleanup_free_ char *comm = NULL; From 2176841b9ec2163f24f0ccdbeb4b9d1981be7c24 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 30 Jul 2024 22:43:49 +0200 Subject: [PATCH 12/13] cgroup-util: clean up cg_kill() and friends, completely split out cg_kill_kernel_sigkill() cg_kill_kernel_sigkill() has a narrow use case, and currently no code really reaches that branch. Let's detach it from cg_kill_recursive() hence, and call it explicitly later where appropriate. --- src/basic/cgroup-util.c | 164 ++++++++++++++++++++-------------------- 1 file changed, 81 insertions(+), 83 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 01af942cab..55a36c4f1b 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -293,27 +293,29 @@ int cg_read_subgroup(DIR *d, char **ret) { static int cg_kill_items( const char *path, + const char *item, int sig, CGroupFlags flags, Set *s, cg_kill_log_func_t log_kill, - void *userdata, - const char *item) { + void *userdata) { _cleanup_set_free_ Set *allocated_set = NULL; - bool done = false; - int r, ret = 0, ret_log_kill = 0; + int r, ret = 0; + assert(path); + assert(item); assert(sig >= 0); - /* Don't send SIGCONT twice. Also, SIGKILL always works even when process is suspended, hence don't send - * SIGCONT on SIGKILL. */ + /* Don't send SIGCONT twice. Also, SIGKILL always works even when process is suspended, hence + * don't send SIGCONT on SIGKILL. */ if (IN_SET(sig, SIGCONT, SIGKILL)) flags &= ~CGROUP_SIGCONT; - /* This goes through the tasks list and kills them all. This - * is repeated until no further processes are added to the - * tasks list, to properly handle forking processes */ + /* This goes through the tasks list and kills them all. This is repeated until no further processes + * are added to the tasks list, to properly handle forking processes. + * + * When sending SIGKILL, prefer cg_kill_kernel_sigkill(), which is fully atomic. */ if (!s) { s = allocated_set = set_new(NULL); @@ -321,8 +323,11 @@ static int cg_kill_items( return -ENOMEM; } + bool done; do { _cleanup_fclose_ FILE *f = NULL; + int ret_log_kill; + done = true; r = cg_enumerate_items(SYSTEMD_CGROUP_CONTROLLER, path, &f, item); @@ -343,7 +348,7 @@ static int cg_kill_items( if ((flags & CGROUP_IGNORE_SELF) && pidref_is_self(&pidref)) continue; - if (set_get(s, PID_TO_PTR(pidref.pid)) == PID_TO_PTR(pidref.pid)) + if (set_contains(s, PID_TO_PTR(pidref.pid))) continue; /* Ignore kernel threads to mimick the behavior of cgroup.kill. */ @@ -396,13 +401,13 @@ int cg_kill( int r, ret; - r = cg_kill_items(path, sig, flags, s, log_kill, userdata, "cgroup.procs"); - if (r < 0) - log_debug_errno(r, "Failed to kill processes in cgroup '%s' item cgroup.procs: %m", path); - if (r < 0 || sig != SIGKILL) - return r; + assert(path); - ret = r; + ret = cg_kill_items(path, "cgroup.procs", sig, flags, s, log_kill, userdata); + if (ret < 0) + return log_debug_errno(ret, "Failed to kill processes in cgroup '%s' item cgroup.procs: %m", path); + if (sig != SIGKILL) + return ret; /* Only in case of killing with SIGKILL and when using cgroupsv2, kill remaining threads manually as a workaround for kernel bug. It was fixed in 5.2-rc5 (c03cd7738a83), backported to 4.19.66 @@ -417,20 +422,76 @@ int cg_kill( * older kernels or without PIDFD_THREAD pidfd_open() fails with EINVAL. Since we might read non * thread group leader IDs from cgroup.threads, we set CGROUP_NO_PIDFD to avoid trying open pidfd's * for them and instead use the regular pid. */ - r = cg_kill_items(path, sig, flags|CGROUP_NO_PIDFD, s, log_kill, userdata, "cgroup.threads"); + r = cg_kill_items(path, "cgroup.threads", sig, flags|CGROUP_NO_PIDFD, s, log_kill, userdata); if (r < 0) return log_debug_errno(r, "Failed to kill processes in cgroup '%s' item cgroup.threads: %m", path); return r > 0 || ret > 0; } -int cg_kill_kernel_sigkill(const char *path) { - /* Kills the cgroup at `path` directly by writing to its cgroup.kill file. This sends SIGKILL to all - * processes in the cgroup and has the advantage of being completely atomic, unlike cg_kill_items(). */ +int cg_kill_recursive( + const char *path, + int sig, + CGroupFlags flags, + Set *s, + cg_kill_log_func_t log_kill, + void *userdata) { + _cleanup_set_free_ Set *allocated_set = NULL; + _cleanup_closedir_ DIR *d = NULL; + int r, ret; + + assert(path); + assert(sig >= 0); + + if (!s) { + s = allocated_set = set_new(NULL); + if (!s) + return -ENOMEM; + } + + ret = cg_kill(path, sig, flags, s, log_kill, userdata); + + r = cg_enumerate_subgroups(SYSTEMD_CGROUP_CONTROLLER, path, &d); + if (r < 0) { + if (r != -ENOENT) + RET_GATHER(ret, log_debug_errno(r, "Failed to enumerate cgroup '%s' subgroups: %m", path)); + + return ret; + } + + for (;;) { + _cleanup_free_ char *fn = NULL, *p = NULL; + + r = cg_read_subgroup(d, &fn); + if (r < 0) { + RET_GATHER(ret, log_debug_errno(r, "Failed to read subgroup from cgroup '%s': %m", path)); + break; + } + if (r == 0) + break; + + p = path_join(empty_to_root(path), fn); + if (!p) + return -ENOMEM; + + r = cg_kill_recursive(p, sig, flags, s, log_kill, userdata); + if (r < 0) + log_debug_errno(r, "Failed to recursively kill processes in cgroup '%s': %m", p); + if (r != 0 && ret >= 0) + ret = r; + } + + return ret; +} + +int cg_kill_kernel_sigkill(const char *path) { _cleanup_free_ char *killfile = NULL; int r; + /* Kills the cgroup at `path` directly by writing to its cgroup.kill file. This sends SIGKILL to all + * processes in the cgroup and has the advantage of being completely atomic, unlike cg_kill_items(). */ + assert(path); if (!cg_kill_supported()) @@ -447,69 +508,6 @@ int cg_kill_kernel_sigkill(const char *path) { return 0; } -int cg_kill_recursive( - const char *path, - int sig, - CGroupFlags flags, - Set *s, - cg_kill_log_func_t log_kill, - void *userdata) { - - int r, ret; - - assert(path); - assert(sig >= 0); - - if (sig == SIGKILL && cg_kill_supported() && - !FLAGS_SET(flags, CGROUP_IGNORE_SELF) && !s && !log_kill) - /* ignore CGROUP_SIGCONT, since this is a no-op alongside SIGKILL */ - ret = cg_kill_kernel_sigkill(path); - else { - _cleanup_set_free_ Set *allocated_set = NULL; - _cleanup_closedir_ DIR *d = NULL; - - if (!s) { - s = allocated_set = set_new(NULL); - if (!s) - return -ENOMEM; - } - - ret = cg_kill(path, sig, flags, s, log_kill, userdata); - - r = cg_enumerate_subgroups(SYSTEMD_CGROUP_CONTROLLER, path, &d); - if (r < 0) { - if (r != -ENOENT) - RET_GATHER(ret, log_debug_errno(r, "Failed to enumerate cgroup '%s' subgroups: %m", path)); - - return ret; - } - - for (;;) { - _cleanup_free_ char *fn = NULL, *p = NULL; - - r = cg_read_subgroup(d, &fn); - if (r < 0) { - RET_GATHER(ret, log_debug_errno(r, "Failed to read subgroup from cgroup '%s': %m", path)); - break; - } - if (r == 0) - break; - - p = path_join(empty_to_root(path), fn); - if (!p) - return -ENOMEM; - - r = cg_kill_recursive(p, sig, flags, s, log_kill, userdata); - if (r < 0) - log_debug_errno(r, "Failed to recursively kill processes in cgroup '%s': %m", p); - if (r != 0 && ret >= 0) - ret = r; - } - } - - return ret; -} - static const char *controller_to_dirname(const char *controller) { assert(controller); From ba16a93db4f8df9c64b99a3c5d13030517e9634b Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 30 Jul 2024 23:03:15 +0200 Subject: [PATCH 13/13] core/unit: utilize cgroup.kill on client request + SIGKILL --- src/core/unit.c | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index c8c34fc0d9..257b0929af 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -3938,7 +3938,7 @@ static int kill_common_log(const PidRef *pid, int signo, void *userdata) { return 1; } -static int kill_or_sigqueue(PidRef* pidref, int signo, int code, int value) { +static int kill_or_sigqueue(PidRef *pidref, int signo, int code, int value) { assert(pidref_is_set(pidref)); assert(SIGNAL_VALID(signo)); @@ -4058,14 +4058,31 @@ int unit_kill( * resource, and we shouldn't allow us to be subjects for such allocation sprees) */ if (IN_SET(whom, KILL_ALL, KILL_ALL_FAIL) && code == SI_USER) { CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (crt && crt->cgroup_path) { _cleanup_set_free_ Set *pid_set = NULL; - /* Exclude the main/control pids from being killed via the cgroup */ - r = unit_pid_set(u, &pid_set); - if (r < 0) - return log_oom(); + if (signo == SIGKILL) { + r = cg_kill_kernel_sigkill(crt->cgroup_path); + if (r >= 0) { + killed = true; + log_unit_info(u, "Killed unit cgroup with SIGKILL on client request."); + goto finish; + } + if (r != -EOPNOTSUPP) { + if (ret >= 0) + sd_bus_error_set_errnof(ret_error, r, + "Failed to kill unit cgroup: %m"); + RET_GATHER(ret, log_unit_warning_errno(u, r, "Failed to kill unit cgroup: %m")); + goto finish; + } + /* Fall back to manual enumeration */ + } else { + /* Exclude the main/control pids from being killed via the cgroup if + * not SIGKILL */ + r = unit_pid_set(u, &pid_set); + if (r < 0) + return log_oom(); + } r = cg_kill_recursive(crt->cgroup_path, signo, 0, pid_set, kill_common_log, u); if (r < 0 && !IN_SET(r, -ESRCH, -ENOENT)) { @@ -4075,18 +4092,16 @@ int unit_kill( "Failed to send signal SIG%s to auxiliary processes: %m", signal_to_string(signo)); - log_unit_warning_errno( - u, r, - "Failed to send signal SIG%s to auxiliary processes on client request: %m", - signal_to_string(signo)); - - RET_GATHER(ret, r); + RET_GATHER(ret, log_unit_warning_errno( + u, r, + "Failed to send signal SIG%s to auxiliary processes on client request: %m", + signal_to_string(signo))); } - killed = killed || r >= 0; } } +finish: /* If the "fail" versions of the operation are requested, then complain if the set of processes we killed is empty */ if (ret >= 0 && !killed && IN_SET(whom, KILL_ALL_FAIL, KILL_CONTROL_FAIL, KILL_MAIN_FAIL)) return sd_bus_error_set_const(ret_error, BUS_ERROR_NO_SUCH_PROCESS, "No matching processes to kill");