From 81504017f462db1ef4ce2c1f617535f261fe01cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Koutn=C3=BD?= Date: Tue, 26 Jan 2021 17:07:00 +0100 Subject: [PATCH 1/3] cgroup: Simplify cg_get_path_and_check The function controller_is_accessible() doesn't do really much in case of the unified hierarchy. Move common parts into cg_get_path_and_check and make controller check v1 specific. This is refactoring only. --- src/basic/cgroup-util.c | 56 ++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 34 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index bb20a12294..fcab1775bd 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -527,41 +527,16 @@ int cg_get_path(const char *controller, const char *path, const char *suffix, ch return 0; } -static int controller_is_accessible(const char *controller) { - int r; +static int controller_is_v1_accessible(const char *controller) { + const char *cc, *dn; assert(controller); - /* Checks whether a specific controller is accessible, - * i.e. its hierarchy mounted. In the unified hierarchy all - * controllers are considered accessible, except for the named - * hierarchies */ + dn = controller_to_dirname(controller); + cc = strjoina("/sys/fs/cgroup/", dn); - if (!cg_controller_is_valid(controller)) - return -EINVAL; - - r = cg_all_unified(); - if (r < 0) - return r; - if (r > 0) { - /* We don't support named hierarchies if we are using - * the unified hierarchy. */ - - if (streq(controller, SYSTEMD_CGROUP_CONTROLLER)) - return 0; - - if (startswith(controller, "name=")) - return -EOPNOTSUPP; - - } else { - const char *cc, *dn; - - dn = controller_to_dirname(controller); - cc = strjoina("/sys/fs/cgroup/", dn); - - if (laccess(cc, F_OK) < 0) - return -errno; - } + if (laccess(cc, F_OK) < 0) + return -errno; return 0; } @@ -572,10 +547,23 @@ int cg_get_path_and_check(const char *controller, const char *path, const char * assert(controller); assert(fs); - /* Check if the specified controller is actually accessible */ - r = controller_is_accessible(controller); + if (!cg_controller_is_valid(controller)) + return -EINVAL; + + r = cg_all_unified(); if (r < 0) return r; + if (r > 0) { + /* In the unified hierarchy all controllers are considered accessible, + * except for the named hierarchies */ + if (startswith(controller, "name=")) + return -EOPNOTSUPP; + } else { + /* Check if the specified controller is actually accessible */ + r = controller_is_v1_accessible(controller); + if (r < 0) + return r; + } return cg_get_path(controller, path, suffix, fs); } @@ -1909,7 +1897,7 @@ int cg_mask_supported(CGroupMask *ret) { continue; n = cgroup_controller_to_string(c); - if (controller_is_accessible(n) >= 0) + if (controller_is_v1_accessible(n) >= 0) mask |= bit; } } From 0fa7b50053695a3012af71c719dd86c12ab10fc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Koutn=C3=BD?= Date: Tue, 26 Jan 2021 19:15:45 +0100 Subject: [PATCH 2/3] core: Make (user) instance aware of delegated cgroup controllers systemd user instance assumed same controllers are available to it as to PID 1. That is not true generally, in v1 (legacy, hybrid) we don't delegate any controllers to anyone and in v2 (unified) we may delegate only subset of controllers. The user instance would fail silently when the controller cgroup cannot be created or the controller cannot be enabled on the unified hierarchy. The changes in 7b63961415 ("cgroup: Swap cgroup v1 deletion and migration") caused some attempts of operating on non-delegated controllers to be logged. Make the user instance first check what controllers are availble to it and narrow operations only to these controllers. The original checks are kept in place. Note that daemon-reexec needs to be invoked in order to update the set of unabled controllers after a change. Fixes: #18047 Fixes: #17862 --- src/basic/cgroup-util.c | 39 ++++++++++++++++++++++++++------------- src/basic/cgroup-util.h | 1 + src/core/cgroup.c | 2 +- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index fcab1775bd..88d6d7a6fb 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -527,15 +527,21 @@ int cg_get_path(const char *controller, const char *path, const char *suffix, ch return 0; } -static int controller_is_v1_accessible(const char *controller) { - const char *cc, *dn; +static int controller_is_v1_accessible(const char *root, const char *controller) { + const char *cpath, *dn; assert(controller); dn = controller_to_dirname(controller); - cc = strjoina("/sys/fs/cgroup/", dn); + cpath = strjoina("/sys/fs/cgroup/", dn); + if (root) + /* Also check that: + * - possible subcgroup is created at root, + * - we can modify the hierarchy. + * "Leak" cpath on stack */ + cpath = strjoina(cpath, root, "/cgroup.procs"); - if (laccess(cc, F_OK) < 0) + if (laccess(cpath, root ? W_OK : F_OK) < 0) return -errno; return 0; @@ -560,7 +566,7 @@ int cg_get_path_and_check(const char *controller, const char *path, const char * return -EOPNOTSUPP; } else { /* Check if the specified controller is actually accessible */ - r = controller_is_v1_accessible(controller); + r = controller_is_v1_accessible(NULL, controller); if (r < 0) return r; } @@ -1847,7 +1853,7 @@ int cg_mask_from_string(const char *value, CGroupMask *ret) { return 0; } -int cg_mask_supported(CGroupMask *ret) { +int cg_mask_supported_subtree(const char *root, CGroupMask *ret) { CGroupMask mask; int r; @@ -1859,15 +1865,11 @@ int cg_mask_supported(CGroupMask *ret) { if (r < 0) return r; if (r > 0) { - _cleanup_free_ char *root = NULL, *controllers = NULL, *path = NULL; + _cleanup_free_ char *controllers = NULL, *path = NULL; /* In the unified hierarchy we can read the supported and accessible controllers from * the top-level cgroup attribute */ - r = cg_get_root_path(&root); - if (r < 0) - return r; - r = cg_get_path(SYSTEMD_CGROUP_CONTROLLER, root, "cgroup.controllers", &path); if (r < 0) return r; @@ -1886,7 +1888,7 @@ int cg_mask_supported(CGroupMask *ret) { } else { CGroupController c; - /* In the legacy hierarchy, we check which hierarchies are mounted. */ + /* In the legacy hierarchy, we check which hierarchies are accessible. */ mask = 0; for (c = 0; c < _CGROUP_CONTROLLER_MAX; c++) { @@ -1897,7 +1899,7 @@ int cg_mask_supported(CGroupMask *ret) { continue; n = cgroup_controller_to_string(c); - if (controller_is_v1_accessible(n) >= 0) + if (controller_is_v1_accessible(root, n) >= 0) mask |= bit; } } @@ -1906,6 +1908,17 @@ int cg_mask_supported(CGroupMask *ret) { return 0; } +int cg_mask_supported(CGroupMask *ret) { + _cleanup_free_ char *root = NULL; + int r; + + r = cg_get_root_path(&root); + if (r < 0) + return r; + + return cg_mask_supported_subtree(root, ret); +} + int cg_kernel_controllers(Set **ret) { _cleanup_set_free_free_ Set *controllers = NULL; _cleanup_fclose_ FILE *f = NULL; diff --git a/src/basic/cgroup-util.h b/src/basic/cgroup-util.h index c5f54aafea..4f1d95620c 100644 --- a/src/basic/cgroup-util.h +++ b/src/basic/cgroup-util.h @@ -257,6 +257,7 @@ 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); int cg_mask_to_string(CGroupMask mask, char **ret); diff --git a/src/core/cgroup.c b/src/core/cgroup.c index c9cf7fb16c..a0d188e1c4 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -3110,7 +3110,7 @@ int manager_setup_cgroup(Manager *m) { (void) cg_set_attribute("memory", "/", "memory.use_hierarchy", "1"); /* 8. Figure out which controllers are supported */ - r = cg_mask_supported(&m->cgroup_supported); + r = cg_mask_supported_subtree(m->cgroup_root, &m->cgroup_supported); if (r < 0) return log_error_errno(r, "Failed to determine supported controllers: %m"); From 873446f2e4133a2fc33557c7b61a8e19b0b3365c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Koutn=C3=BD?= Date: Wed, 10 Feb 2021 18:47:13 +0100 Subject: [PATCH 3/3] Revert "Silence cgroups v1 read-only filesystem warning" PID 1 will now check upfront which v1 controller hiearchies are available and modifiable and therefore it will not attempt to touch them. If we get an EROFS failure then, it points to another inconsistency so we will report it again. The revert also simplifies the code a bit. --- src/core/cgroup.c | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index a0d188e1c4..1347e51904 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1839,10 +1839,6 @@ int unit_pick_cgroup_path(Unit *u) { return 0; } -static int cg_v1_errno_to_log_level(int r) { - return r == -EROFS ? LOG_DEBUG : LOG_WARNING; -} - static int unit_update_cgroup( Unit *u, CGroupMask target_mask, @@ -1900,30 +1896,16 @@ static int unit_update_cgroup( * We perform migration also with whole slices for cases when users don't care about leave * granularity. Since delegated_mask is subset of target mask, we won't trim slice subtree containing * delegated units. - * - * If we're in an nspawn container and using legacy cgroups, the controller hierarchies are mounted - * read-only into the container. We skip migration/trim in this scenario since it would fail - * regardless with noisy "Read-only filesystem" warnings. */ if (cg_all_unified() == 0) { r = cg_migrate_v1_controllers(u->manager->cgroup_supported, migrate_mask, u->cgroup_path, migrate_callback, u); if (r < 0) - log_unit_full_errno( - u, - cg_v1_errno_to_log_level(r), - r, - "Failed to migrate controller cgroups from %s, ignoring: %m", - u->cgroup_path); + log_unit_warning_errno(u, r, "Failed to migrate controller cgroups from %s, ignoring: %m", u->cgroup_path); is_root_slice = unit_has_name(u, SPECIAL_ROOT_SLICE); r = cg_trim_v1_controllers(u->manager->cgroup_supported, ~target_mask, u->cgroup_path, !is_root_slice); if (r < 0) - log_unit_full_errno( - u, - cg_v1_errno_to_log_level(r), - r, - "Failed to delete controller cgroups %s, ignoring: %m", - u->cgroup_path); + log_unit_warning_errno(u, r, "Failed to delete controller cgroups %s, ignoring: %m", u->cgroup_path); } /* Set attributes */