From b1cd8ed42094f64a201c225be8d2c648f9067444 Mon Sep 17 00:00:00 2001 From: Nick Rosbrook Date: Tue, 14 Jun 2022 15:25:50 -0400 Subject: [PATCH 1/5] core/cgroup: allow setting oomd xattrs from user manager The extended attributes 'user.oomd_avoid' and 'user.oomd_omit' are used to store the setting of the ManagedOOMPrefernce property. Since these are user extended attributes, the access permissions are defined by the file permissions, and there is no need to guard this from user managers. --- src/core/cgroup.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 746c7cdfed..c945f76abc 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -783,6 +783,9 @@ static void cgroup_xattr_apply(Unit *u) { assert(u); + /* The 'user.*' xattrs can be set from a user manager. */ + cgroup_oomd_xattr_apply(u, u->cgroup_path); + if (!MANAGER_IS_SYSTEM(u->manager)) return; @@ -809,8 +812,6 @@ static void cgroup_xattr_apply(Unit *u) { else unit_remove_xattr_graceful(u, NULL, xn); } - - cgroup_oomd_xattr_apply(u, u->cgroup_path); } static int lookup_block_device(const char *p, dev_t *ret) { From 652a4efb66a845f6c5d48429aa4df0194a602aaa Mon Sep 17 00:00:00 2001 From: Nick Rosbrook Date: Tue, 19 Jul 2022 11:49:06 -0400 Subject: [PATCH 2/5] oomd: loosen the restriction on ManagedOOMPreference The ManagedOOMPreference property is only honored on cgroups which are owned by root. This precludes anyone from setting ManagedOOMPreference on cgroups managed by user managers. Loosen this restriction in the following way: when processing a monitored cgroup for kill candidates, honor the ManagedOOMPreference setting if the monitored cgroup and cgroup candidate are owned by the same user. This allows unprivileged users to configure ManagedOOMPreference on their cgroups without affecting the kill priority of ancestor cgroups. N.B. that since swap kill operates globally to kill the largest candidate, it is not appropriate to apply this logic to the swap kill scenario. Therefore, the existing restriction on ManagedOOMPreference will remain when calculating candidates for swap kill. Add a new function, oomd_fetch_cgroup_oom_preference, to assist with this new logic. To simplify things, move the `user.oomd_{avoid,omit}` xattr reads to this function so that the xattr reads and uid checks are performed all at once. --- src/oom/oomd-util.c | 71 +++++++++++++++++++++++++++++++++------------ src/oom/oomd-util.h | 9 ++++++ 2 files changed, 61 insertions(+), 19 deletions(-) diff --git a/src/oom/oomd-util.c b/src/oom/oomd-util.c index d1ab528a0e..8a3fb09e37 100644 --- a/src/oom/oomd-util.c +++ b/src/oom/oomd-util.c @@ -14,6 +14,7 @@ #include "sort-util.h" #include "stat-util.h" #include "stdio-util.h" +#include "user-util.h" DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR( oomd_cgroup_ctx_hash_ops, @@ -142,10 +143,53 @@ bool oomd_swap_free_below(const OomdSystemContext *ctx, int threshold_permyriad) return (ctx->swap_total - ctx->swap_used) < swap_threshold; } +int oomd_fetch_cgroup_oom_preference(OomdCGroupContext *ctx, const char *prefix) { + uid_t uid, prefix_uid; + int r; + + assert(ctx); + + prefix = empty_to_root(prefix); + + if (!path_startswith(ctx->path, prefix)) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), + "%s is not a descendant of %s", ctx->path, prefix); + + r = cg_get_owner(SYSTEMD_CGROUP_CONTROLLER, ctx->path, &uid); + if (r < 0) + return log_debug_errno(r, "Failed to get owner/group from %s: %m", ctx->path); + + r = cg_get_owner(SYSTEMD_CGROUP_CONTROLLER, prefix, &prefix_uid); + if (r < 0) + return log_debug_errno(r, "Failed to get owner/group from %s: %m", ctx->path); + + if (uid == prefix_uid) { + /* Ignore most errors when reading the xattr since it is usually unset and cgroup xattrs are only used + * as an optional feature of systemd-oomd (and the system might not even support them). */ + r = cg_get_xattr_bool(SYSTEMD_CGROUP_CONTROLLER, ctx->path, "user.oomd_avoid"); + if (r == -ENOMEM) + return log_oom_debug(); + if (r < 0 && r != -ENODATA) + log_debug_errno(r, "Failed to get xattr user.oomd_avoid, ignoring: %m"); + ctx->preference = r > 0 ? MANAGED_OOM_PREFERENCE_AVOID : ctx->preference; + + r = cg_get_xattr_bool(SYSTEMD_CGROUP_CONTROLLER, ctx->path, "user.oomd_omit"); + if (r == -ENOMEM) + return log_oom_debug(); + if (r < 0 && r != -ENODATA) + log_debug_errno(r, "Failed to get xattr user.oomd_omit, ignoring: %m"); + ctx->preference = r > 0 ? MANAGED_OOM_PREFERENCE_OMIT : ctx->preference; + } else + ctx->preference = MANAGED_OOM_PREFERENCE_NONE; + + return 0; +} + int oomd_sort_cgroup_contexts(Hashmap *h, oomd_compare_t compare_func, const char *prefix, OomdCGroupContext ***ret) { _cleanup_free_ OomdCGroupContext **sorted = NULL; OomdCGroupContext *item; size_t k = 0; + int r; assert(h); assert(compare_func); @@ -157,7 +201,14 @@ int oomd_sort_cgroup_contexts(Hashmap *h, oomd_compare_t compare_func, const cha HASHMAP_FOREACH(item, h) { /* Skip over cgroups that are not valid candidates or are explicitly marked for omission */ - if ((item->path && prefix && !path_startswith(item->path, prefix)) || item->preference == MANAGED_OOM_PREFERENCE_OMIT) + if (item->path && prefix && !path_startswith(item->path, prefix)) + continue; + + r = oomd_fetch_cgroup_oom_preference(item, prefix); + if (r == -ENOMEM) + return r; + + if (item->preference == MANAGED_OOM_PREFERENCE_OMIT) continue; sorted[k++] = item; @@ -334,7 +385,6 @@ int oomd_cgroup_context_acquire(const char *path, OomdCGroupContext **ret) { _cleanup_(oomd_cgroup_context_freep) OomdCGroupContext *ctx = NULL; _cleanup_free_ char *p = NULL, *val = NULL; bool is_root; - uid_t uid; int r; assert(path); @@ -355,23 +405,6 @@ int oomd_cgroup_context_acquire(const char *path, OomdCGroupContext **ret) { if (r < 0) return log_debug_errno(r, "Error parsing memory pressure from %s: %m", p); - r = cg_get_owner(SYSTEMD_CGROUP_CONTROLLER, path, &uid); - if (r < 0) - log_debug_errno(r, "Failed to get owner/group from %s: %m", path); - else if (uid == 0) { - /* Ignore most errors when reading the xattr since it is usually unset and cgroup xattrs are only used - * as an optional feature of systemd-oomd (and the system might not even support them). */ - r = cg_get_xattr_bool(SYSTEMD_CGROUP_CONTROLLER, path, "user.oomd_avoid"); - if (r == -ENOMEM) - return r; - ctx->preference = r == 1 ? MANAGED_OOM_PREFERENCE_AVOID : ctx->preference; - - r = cg_get_xattr_bool(SYSTEMD_CGROUP_CONTROLLER, path, "user.oomd_omit"); - if (r == -ENOMEM) - return r; - ctx->preference = r == 1 ? MANAGED_OOM_PREFERENCE_OMIT : ctx->preference; - } - if (is_root) { r = procfs_memory_get_used(&ctx->current_memory_usage); if (r < 0) diff --git a/src/oom/oomd-util.h b/src/oom/oomd-util.h index de2298f3c9..7fd9e92109 100644 --- a/src/oom/oomd-util.h +++ b/src/oom/oomd-util.h @@ -109,6 +109,15 @@ static inline int compare_swap_usage(OomdCGroupContext * const *c1, OomdCGroupCo * Returns the number of sorted items; negative on error. */ int oomd_sort_cgroup_contexts(Hashmap *h, oomd_compare_t compare_func, const char *prefix, OomdCGroupContext ***ret); +/* If the cgroups represented by `ctx` and `prefix` are owned by the same user, + * then set `ctx->preference` using the `user.oomd_avoid` and `user.oomd_omit` + * xattrs. Otherwise, set `ctx->preference` to MANAGED_OOM_PREFERENCE_NONE. + * + * If `prefix` is NULL or the empty string, it is treated as root. If `prefix` + * does not specify an ancestor cgroup of `ctx`, -EINVAL is returned. Returns + * negative on all other errors. */ +int oomd_fetch_cgroup_oom_preference(OomdCGroupContext *ctx, const char *prefix); + /* Returns a negative value on error, 0 if no processes were killed, or 1 if processes were killed. */ int oomd_cgroup_kill(const char *path, bool recurse, bool dry_run); From 39f9eee882b2a12410db07a24b8538957abf358b Mon Sep 17 00:00:00 2001 From: Nick Rosbrook Date: Tue, 19 Jul 2022 12:48:04 -0400 Subject: [PATCH 3/5] oomd: add tests for oomd_fetch_cgroup_oom_preference Add a new test function, test_oomd_fetch_cgroup_oom_preference, to test the ManagedOOMPreference logic. For starters, cut the relevant tests out of test_oomd_cgroup_context_acquire_and_insert, and add them to the new function. Then, expand these tests to cover the new behavior. --- src/oom/test-oomd-util.c | 128 +++++++++++++++++++++++++-------------- 1 file changed, 83 insertions(+), 45 deletions(-) diff --git a/src/oom/test-oomd-util.c b/src/oom/test-oomd-util.c index 19173edcda..176e3a8d69 100644 --- a/src/oom/test-oomd-util.c +++ b/src/oom/test-oomd-util.c @@ -94,11 +94,8 @@ static void test_oomd_cgroup_context_acquire_and_insert(void) { _cleanup_hashmap_free_ Hashmap *h1 = NULL, *h2 = NULL; _cleanup_(oomd_cgroup_context_freep) OomdCGroupContext *ctx = NULL; _cleanup_free_ char *cgroup = NULL; - ManagedOOMPreference root_pref; OomdCGroupContext *c1, *c2; CGroupMask mask; - bool test_xattrs; - int root_xattrs, r; if (geteuid() != 0) return (void) log_tests_skipped("not root"); @@ -115,17 +112,6 @@ static void test_oomd_cgroup_context_acquire_and_insert(void) { return (void) log_tests_skipped("cgroup memory controller is not available"); assert_se(cg_pid_get_path(NULL, 0, &cgroup) >= 0); - - /* If we don't have permissions to set xattrs we're likely in a userns or missing capabilities - * so skip the xattr portions of the test. */ - r = cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup, "user.oomd_test", "1", 1, 0); - test_xattrs = !ERRNO_IS_PRIVILEGE(r) && !ERRNO_IS_NOT_SUPPORTED(r); - - if (test_xattrs) { - assert_se(cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup, "user.oomd_omit", "1", 1, 0) >= 0); - assert_se(cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup, "user.oomd_avoid", "1", 1, 0) >= 0); - } - assert_se(oomd_cgroup_context_acquire(cgroup, &ctx) == 0); assert_se(streq(ctx->path, cgroup)); @@ -135,34 +121,11 @@ static void test_oomd_cgroup_context_acquire_and_insert(void) { assert_se(ctx->swap_usage == 0); assert_se(ctx->last_pgscan == 0); assert_se(ctx->pgscan == 0); - /* omit takes precedence over avoid when both are set to true */ - if (test_xattrs) - assert_se(ctx->preference == MANAGED_OOM_PREFERENCE_OMIT); - else - assert_se(ctx->preference == MANAGED_OOM_PREFERENCE_NONE); ctx = oomd_cgroup_context_free(ctx); - /* also check when only avoid is set to true */ - if (test_xattrs) { - assert_se(cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup, "user.oomd_omit", "0", 1, 0) >= 0); - assert_se(cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup, "user.oomd_avoid", "1", 1, 0) >= 0); - } - assert_se(oomd_cgroup_context_acquire(cgroup, &ctx) == 0); - if (test_xattrs) - assert_se(ctx->preference == MANAGED_OOM_PREFERENCE_AVOID); - ctx = oomd_cgroup_context_free(ctx); - - /* Test the root cgroup */ - /* Root cgroup is live and not made on demand like the cgroup the test runs in. It can have varying - * xattrs set already so let's read in the booleans first to get the final preference value. */ - root_xattrs = cg_get_xattr_bool(SYSTEMD_CGROUP_CONTROLLER, "", "user.oomd_omit"); - root_pref = root_xattrs > 0 ? MANAGED_OOM_PREFERENCE_OMIT : MANAGED_OOM_PREFERENCE_NONE; - root_xattrs = cg_get_xattr_bool(SYSTEMD_CGROUP_CONTROLLER, "", "user.oomd_avoid"); - root_pref = root_xattrs > 0 ? MANAGED_OOM_PREFERENCE_AVOID : MANAGED_OOM_PREFERENCE_NONE; assert_se(oomd_cgroup_context_acquire("", &ctx) == 0); assert_se(streq(ctx->path, "/")); assert_se(ctx->current_memory_usage > 0); - assert_se(ctx->preference == root_pref); /* Test hashmap inserts */ assert_se(h1 = hashmap_new(&oomd_cgroup_ctx_hash_ops)); @@ -187,14 +150,6 @@ static void test_oomd_cgroup_context_acquire_and_insert(void) { assert_se(c2->mem_pressure_limit == 6789); assert_se(c2->mem_pressure_limit_hit_start == 42); assert_se(c2->last_had_mem_reclaim == 888); /* assumes the live pgscan is less than UINT64_MAX */ - - /* Assert that avoid/omit are not set if the cgroup is not owned by root */ - if (test_xattrs) { - ctx = oomd_cgroup_context_free(ctx); - assert_se(cg_set_access(SYSTEMD_CGROUP_CONTROLLER, cgroup, 65534, 0) >= 0); - assert_se(oomd_cgroup_context_acquire(cgroup, &ctx) == 0); - assert_se(ctx->preference == MANAGED_OOM_PREFERENCE_NONE); - } } static void test_oomd_update_cgroup_contexts_between_hashmaps(void) { @@ -451,6 +406,88 @@ static void test_oomd_sort_cgroups(void) { sorted_cgroups = mfree(sorted_cgroups); } +static void test_oomd_fetch_cgroup_oom_preference(void) { + _cleanup_(oomd_cgroup_context_freep) OomdCGroupContext *ctx = NULL; + _cleanup_free_ char *cgroup = NULL; + ManagedOOMPreference root_pref; + CGroupMask mask; + bool test_xattrs; + int root_xattrs, r; + + if (geteuid() != 0) + return (void) log_tests_skipped("not root"); + + if (!is_pressure_supported()) + return (void) log_tests_skipped("system does not support pressure"); + + if (cg_all_unified() <= 0) + return (void) log_tests_skipped("cgroups are not running in unified mode"); + + assert_se(cg_mask_supported(&mask) >= 0); + + if (!FLAGS_SET(mask, CGROUP_MASK_MEMORY)) + return (void) log_tests_skipped("cgroup memory controller is not available"); + + assert_se(cg_pid_get_path(NULL, 0, &cgroup) >= 0); + assert_se(oomd_cgroup_context_acquire(cgroup, &ctx) == 0); + + /* If we don't have permissions to set xattrs we're likely in a userns or missing capabilities + * so skip the xattr portions of the test. */ + r = cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup, "user.oomd_test", "1", 1, 0); + test_xattrs = !ERRNO_IS_PRIVILEGE(r) && !ERRNO_IS_NOT_SUPPORTED(r); + + if (test_xattrs) { + assert_se(oomd_fetch_cgroup_oom_preference(ctx, NULL) == 0); + assert_se(cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup, "user.oomd_omit", "1", 1, 0) >= 0); + assert_se(cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup, "user.oomd_avoid", "1", 1, 0) >= 0); + + /* omit takes precedence over avoid when both are set to true */ + assert_se(oomd_fetch_cgroup_oom_preference(ctx, NULL) == 0); + assert_se(ctx->preference == MANAGED_OOM_PREFERENCE_OMIT); + } else { + assert_se(oomd_fetch_cgroup_oom_preference(ctx, NULL) < 0); + assert_se(ctx->preference == MANAGED_OOM_PREFERENCE_NONE); + } + ctx = oomd_cgroup_context_free(ctx); + + /* also check when only avoid is set to true */ + if (test_xattrs) { + assert_se(cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup, "user.oomd_omit", "0", 1, 0) >= 0); + assert_se(cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup, "user.oomd_avoid", "1", 1, 0) >= 0); + assert_se(oomd_cgroup_context_acquire(cgroup, &ctx) == 0); + assert_se(oomd_fetch_cgroup_oom_preference(ctx, NULL) == 0); + assert_se(ctx->preference == MANAGED_OOM_PREFERENCE_AVOID); + ctx = oomd_cgroup_context_free(ctx); + } + + /* Test the root cgroup */ + /* Root cgroup is live and not made on demand like the cgroup the test runs in. It can have varying + * xattrs set already so let's read in the booleans first to get the final preference value. */ + assert_se(oomd_cgroup_context_acquire("", &ctx) == 0); + root_xattrs = cg_get_xattr_bool(SYSTEMD_CGROUP_CONTROLLER, "", "user.oomd_omit"); + root_pref = root_xattrs > 0 ? MANAGED_OOM_PREFERENCE_OMIT : MANAGED_OOM_PREFERENCE_NONE; + root_xattrs = cg_get_xattr_bool(SYSTEMD_CGROUP_CONTROLLER, "", "user.oomd_avoid"); + root_pref = root_xattrs > 0 ? MANAGED_OOM_PREFERENCE_AVOID : MANAGED_OOM_PREFERENCE_NONE; + assert_se(oomd_fetch_cgroup_oom_preference(ctx, NULL) == 0); + assert_se(ctx->preference == root_pref); + + assert_se(oomd_fetch_cgroup_oom_preference(ctx, "/herp.slice/derp.scope") == -EINVAL); + + /* Assert that avoid/omit are not set if the cgroup and prefix are not + * owned by the same user.*/ + if (test_xattrs && !empty_or_root(ctx->path)) { + ctx = oomd_cgroup_context_free(ctx); + assert_se(cg_set_access(SYSTEMD_CGROUP_CONTROLLER, cgroup, 65534, 0) >= 0); + assert_se(oomd_cgroup_context_acquire(cgroup, &ctx) == 0); + + assert_se(oomd_fetch_cgroup_oom_preference(ctx, NULL) == 0); + assert_se(ctx->preference == MANAGED_OOM_PREFERENCE_NONE); + + assert_se(oomd_fetch_cgroup_oom_preference(ctx, ctx->path) == 0); + assert_se(ctx->preference == MANAGED_OOM_PREFERENCE_AVOID); + } +} + int main(void) { int r; @@ -470,6 +507,7 @@ int main(void) { test_oomd_cgroup_kill(); test_oomd_cgroup_context_acquire_and_insert(); + test_oomd_fetch_cgroup_oom_preference(); return 0; } From 58b2f0d1f578f4209e97f1f6c4bbe9c7c2aeb334 Mon Sep 17 00:00:00 2001 From: Nick Rosbrook Date: Thu, 7 Jul 2022 17:17:53 -0400 Subject: [PATCH 4/5] man: update ManagedOOMPreference entry to reflect new behavior --- man/systemd.resource-control.xml | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/man/systemd.resource-control.xml b/man/systemd.resource-control.xml index 940395a442..2b1c20b96e 100644 --- a/man/systemd.resource-control.xml +++ b/man/systemd.resource-control.xml @@ -1077,9 +1077,20 @@ DeviceAllow=/dev/loop-control Allows deprioritizing or omitting this unit's cgroup as a candidate when systemd-oomd needs to act. Requires support for extended attributes (see xattr7) - in order to use or . Additionally, - systemd-oomd will ignore these extended attributes if the unit's cgroup is not - owned by the root user. + in order to use or . + + When calculating candidates to relieve swap usage, systemd-oomd will + only respect these extended attributes if the unit's cgroup is owned by root. + + When calculating candidates to relieve memory pressure, systemd-oomd + will only respect these extended attributes if the unit's cgroup owner, and the + owner of the monitored ancestor cgroup are the same. For example, if systemd-oomd + is calculating candidates for -.slice, then extended attributes set + on descendants of /user.slice/user-1000.slice/user@1000.service/ + will be ignored because the descendants are owned by UID 1000, and -.slice + is owned by UID 0. But, if calculating candidates for + /user.slice/user-1000.slice/user@1000.service/, then extended attributes set + on the descendants would be respected. If this property is set to , the service manager will convey this to systemd-oomd, which will only select this cgroup if there are no other viable From b814de305445f59baa289ab926cbe6ca05ac26a4 Mon Sep 17 00:00:00 2001 From: Nick Rosbrook Date: Wed, 20 Jul 2022 11:16:37 -0400 Subject: [PATCH 5/5] oomd: fix off-by-one when dumping kill candidates When we kill a cgroup that is towards the end of the sorted candidate list (i.e. when we have to resort to killing a candidate with ManagedOOMPreference=avoid), this cgroup is not logged in the candidate list. This is due to an off-by-one error when assigning dump_until. --- src/oom/oomd-util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/oom/oomd-util.c b/src/oom/oomd-util.c index 8a3fb09e37..d31d5ca623 100644 --- a/src/oom/oomd-util.c +++ b/src/oom/oomd-util.c @@ -323,7 +323,7 @@ int oomd_kill_by_pgscan_rate(Hashmap *h, const char *prefix, bool dry_run, char continue; /* Try to find something else to kill */ } - dump_until = MAX(dump_until, i); + dump_until = MAX(dump_until, i + 1); char *selected = strdup(sorted[i]->path); if (!selected) return -ENOMEM; @@ -367,7 +367,7 @@ int oomd_kill_by_swap_usage(Hashmap *h, uint64_t threshold_usage, bool dry_run, continue; /* Try to find something else to kill */ } - dump_until = MAX(dump_until, i); + dump_until = MAX(dump_until, i + 1); char *selected = strdup(sorted[i]->path); if (!selected) return -ENOMEM;