From 16b6af6adefa4068b10e1a04f46fc895cb9a3af7 Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Sun, 21 Jan 2024 15:05:20 -0500 Subject: [PATCH 1/2] core: Rework recursive freeze/thaw This commit overhauls the way freeze/thaw works recursively: First, it introduces new FreezerActions that are like the existing FREEZE and THAW but indicate that the action was initiated by a parent unit. We also refactored the code to pass these FreezerActions through the whole call stack so that we can make use of them. FreezerState was extended similarly, to be able to differentiate between a unit that's frozen manually and a unit that's frozen because a parent is frozen. Next, slices were changed to check recursively that all their child units can be frozen before it attempts to freeze them. This is different from the previous behavior, that would just check if the unit's type supported freezing at all. This cleans up the code, and also ensures that the behavior of slices corresponds to the unit's actual ability to be frozen Next, we make it so that if you FREEZE a slice, it'll PARENT_FREEZE all of its children. Similarly, if you THAW a slice it will PARENT_THAW its children. Finally, we use the new states available to us to refactor the code that actually does the cgroup freezing. The code now looks at the unit's existing freezer state and the action being requested, and decides what next state is most appropriate. Then it puts the unit in that state. For instance, a RUNNING unit with a request to PARENT_FREEZE will put the unit into the PARENT_FREEZING state. As another example, a FROZEN unit who's parent is also FROZEN will transition to PARENT_FROZEN in response to a request to THAW. Fixes https://github.com/systemd/systemd/issues/30640 Fixes https://github.com/systemd/systemd/issues/15850 --- src/basic/unit-def.c | 27 ++++++- src/basic/unit-def.h | 5 +- src/core/cgroup.c | 122 +++++++++++++++++-------------- src/core/cgroup.h | 5 +- src/core/dbus-unit.c | 17 ++--- src/core/scope.c | 3 +- src/core/service.c | 3 +- src/core/slice.c | 62 ++++++---------- src/core/unit.c | 146 +++++++++++++++++++++++-------------- src/core/unit.h | 15 ++-- test/units/testsuite-38.sh | 133 ++++++++++++++++++++++++++------- 11 files changed, 332 insertions(+), 206 deletions(-) diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c index 908c0cd03f..9bc4920998 100644 --- a/src/basic/unit-def.c +++ b/src/basic/unit-def.c @@ -117,14 +117,33 @@ static const char* const unit_active_state_table[_UNIT_ACTIVE_STATE_MAX] = { DEFINE_STRING_TABLE_LOOKUP(unit_active_state, UnitActiveState); static const char* const freezer_state_table[_FREEZER_STATE_MAX] = { - [FREEZER_RUNNING] = "running", - [FREEZER_FREEZING] = "freezing", - [FREEZER_FROZEN] = "frozen", - [FREEZER_THAWING] = "thawing", + [FREEZER_RUNNING] = "running", + [FREEZER_FREEZING] = "freezing", + [FREEZER_FREEZING_BY_PARENT] = "freezing-by-parent", + [FREEZER_FROZEN] = "frozen", + [FREEZER_FROZEN_BY_PARENT] = "frozen-by-parent", + [FREEZER_THAWING] = "thawing", }; DEFINE_STRING_TABLE_LOOKUP(freezer_state, FreezerState); +/* Maps in-progress freezer states to the corresponding finished state */ +static const FreezerState freezer_state_finish_table[_FREEZER_STATE_MAX] = { + [FREEZER_FREEZING] = FREEZER_FROZEN, + [FREEZER_FREEZING_BY_PARENT] = FREEZER_FROZEN_BY_PARENT, + [FREEZER_THAWING] = FREEZER_RUNNING, + + /* Finished states trivially map to themselves */ + [FREEZER_RUNNING] = FREEZER_RUNNING, + [FREEZER_FROZEN] = FREEZER_FROZEN, + [FREEZER_FROZEN_BY_PARENT] = FREEZER_FROZEN_BY_PARENT, +}; + +FreezerState freezer_state_finish(FreezerState state) { + assert(state >= 0 && state < _FREEZER_STATE_MAX); + return freezer_state_finish_table[state]; +} + static const char* const unit_marker_table[_UNIT_MARKER_MAX] = { [UNIT_MARKER_NEEDS_RELOAD] = "needs-reload", [UNIT_MARKER_NEEDS_RESTART] = "needs-restart", diff --git a/src/basic/unit-def.h b/src/basic/unit-def.h index 6627da5614..8e73e286da 100644 --- a/src/basic/unit-def.h +++ b/src/basic/unit-def.h @@ -53,8 +53,10 @@ typedef enum UnitActiveState { typedef enum FreezerState { FREEZER_RUNNING, - FREEZER_FREEZING, + FREEZER_FREEZING, /* freezing due to user request */ FREEZER_FROZEN, + FREEZER_FREEZING_BY_PARENT, /* freezing as a result of parent slice freezing */ + FREEZER_FROZEN_BY_PARENT, FREEZER_THAWING, _FREEZER_STATE_MAX, _FREEZER_STATE_INVALID = -EINVAL, @@ -297,6 +299,7 @@ UnitActiveState unit_active_state_from_string(const char *s) _pure_; const char *freezer_state_to_string(FreezerState i) _const_; FreezerState freezer_state_from_string(const char *s) _pure_; +FreezerState freezer_state_finish(FreezerState i) _const_; const char *unit_marker_to_string(UnitMarker m) _const_; UnitMarker unit_marker_from_string(const char *s) _pure_; diff --git a/src/core/cgroup.c b/src/core/cgroup.c index e7585c3c01..5520118026 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -3897,8 +3897,10 @@ static int unit_check_cgroup_events(Unit *u) { unit_add_to_cgroup_empty_queue(u); } - /* Disregard freezer state changes due to operations not initiated by us */ - if (values[1] && IN_SET(u->freezer_state, FREEZER_FREEZING, FREEZER_THAWING)) { + /* Disregard freezer state changes due to operations not initiated by us. + * See: https://github.com/systemd/systemd/pull/13512/files#r416469963 and + * https://github.com/systemd/systemd/pull/13512#issuecomment-573007207 */ + if (values[1] && IN_SET(u->freezer_state, FREEZER_FREEZING, FREEZER_FREEZING_BY_PARENT, FREEZER_THAWING)) { if (streq(values[1], "0")) unit_thawed(u); else @@ -4891,66 +4893,87 @@ void manager_invalidate_startup_units(Manager *m) { unit_invalidate_cgroup(u, CGROUP_MASK_CPU|CGROUP_MASK_IO|CGROUP_MASK_BLKIO|CGROUP_MASK_CPUSET); } -int unit_cgroup_freezer_action(Unit *u, FreezerAction action) { - _cleanup_free_ char *path = NULL; - FreezerState target, kernel = _FREEZER_STATE_INVALID; - int r, ret; +static int unit_cgroup_freezer_kernel_state(Unit *u, FreezerState *ret) { + _cleanup_free_ char *val = NULL; + FreezerState s; + int r; assert(u); - assert(IN_SET(action, FREEZER_FREEZE, FREEZER_THAW)); + assert(ret); - if (!cg_freezer_supported()) - return 0; + r = cg_get_keyed_attribute(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, "cgroup.events", + STRV_MAKE("frozen"), &val); + if (IN_SET(r, -ENOENT, -ENXIO)) + return -ENODATA; + if (r < 0) + return r; - /* Ignore all requests to thaw init.scope or -.slice and reject all requests to freeze them */ - if (unit_has_name(u, SPECIAL_ROOT_SLICE) || unit_has_name(u, SPECIAL_INIT_SCOPE)) - return action == FREEZER_FREEZE ? -EPERM : 0; - - if (!u->cgroup_realized) - return -EBUSY; - - if (action == FREEZER_THAW) { - Unit *slice = UNIT_GET_SLICE(u); - - if (slice) { - r = unit_cgroup_freezer_action(slice, FREEZER_THAW); - if (r < 0) - return log_unit_error_errno(u, r, "Failed to thaw slice %s of unit: %m", slice->id); - } + if (streq(val, "0")) + s = FREEZER_RUNNING; + else if (streq(val, "1")) + s = FREEZER_FROZEN; + else { + log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL), "Unexpected cgroup frozen state: %s", val); + s = _FREEZER_STATE_INVALID; } - target = action == FREEZER_FREEZE ? FREEZER_FROZEN : FREEZER_RUNNING; + *ret = s; + return 0; +} - r = unit_freezer_state_kernel(u, &kernel); +int unit_cgroup_freezer_action(Unit *u, FreezerAction action) { + _cleanup_free_ char *path = NULL; + FreezerState target, current, next; + int r; + + assert(u); + assert(IN_SET(action, FREEZER_FREEZE, FREEZER_PARENT_FREEZE, + FREEZER_THAW, FREEZER_PARENT_THAW)); + + if (!cg_freezer_supported() || !u->cgroup_realized) + return 0; + + unit_next_freezer_state(u, action, &next, &target); + + r = unit_cgroup_freezer_kernel_state(u, ¤t); if (r < 0) - log_unit_debug_errno(u, r, "Failed to obtain cgroup freezer state: %m"); + return r; - if (target == kernel) { - u->freezer_state = target; - if (action == FREEZER_FREEZE) - return 0; - ret = 0; - } else - ret = 1; + if (current == target) + next = freezer_state_finish(next); + else if (IN_SET(next, FREEZER_FROZEN, FREEZER_FROZEN_BY_PARENT, FREEZER_RUNNING)) { + /* We're transitioning into a finished state, which implies that the cgroup's + * current state already matches the target and thus we'd return 0. But, reality + * shows otherwise. This indicates that our freezer_state tracking has diverged + * from the real state of the cgroup, which can happen if someone meddles with the + * cgroup from underneath us. This really shouldn't happen during normal operation, + * though. So, let's warn about it and fix up the state to be valid */ + + log_unit_warning(u, "Unit wants to transition to %s freezer state but cgroup is unexpectedly %s, fixing up.", + freezer_state_to_string(next), freezer_state_to_string(current) ?: "(invalid)"); + + if (next == FREEZER_FROZEN) + next = FREEZER_FREEZING; + else if (next == FREEZER_FROZEN_BY_PARENT) + next = FREEZER_FREEZING_BY_PARENT; + else if (next == FREEZER_RUNNING) + next = FREEZER_THAWING; + } r = cg_get_path(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, "cgroup.freeze", &path); if (r < 0) return r; - log_unit_debug(u, "%s unit.", action == FREEZER_FREEZE ? "Freezing" : "Thawing"); + log_unit_debug(u, "Unit freezer state was %s, now %s.", + freezer_state_to_string(u->freezer_state), + freezer_state_to_string(next)); - if (target != kernel) { - if (action == FREEZER_FREEZE) - u->freezer_state = FREEZER_FREEZING; - else - u->freezer_state = FREEZER_THAWING; - } - - r = write_string_file(path, one_zero(action == FREEZER_FREEZE), WRITE_STRING_FILE_DISABLE_BUFFER); + r = write_string_file(path, one_zero(target == FREEZER_FROZEN), WRITE_STRING_FILE_DISABLE_BUFFER); if (r < 0) return r; - return ret; + u->freezer_state = next; + return target != current; } int unit_get_cpuset(Unit *u, CPUSet *cpus, const char *name) { @@ -4989,17 +5012,10 @@ static const char* const cgroup_device_policy_table[_CGROUP_DEVICE_POLICY_MAX] = DEFINE_STRING_TABLE_LOOKUP(cgroup_device_policy, CGroupDevicePolicy); -static const char* const freezer_action_table[_FREEZER_ACTION_MAX] = { - [FREEZER_FREEZE] = "freeze", - [FREEZER_THAW] = "thaw", -}; - -DEFINE_STRING_TABLE_LOOKUP(freezer_action, FreezerAction); - static const char* const cgroup_pressure_watch_table[_CGROUP_PRESSURE_WATCH_MAX] = { - [CGROUP_PRESSURE_WATCH_OFF] = "off", + [CGROUP_PRESSURE_WATCH_OFF] = "off", [CGROUP_PRESSURE_WATCH_AUTO] = "auto", - [CGROUP_PRESSURE_WATCH_ON] = "on", + [CGROUP_PRESSURE_WATCH_ON] = "on", [CGROUP_PRESSURE_WATCH_SKIP] = "skip", }; diff --git a/src/core/cgroup.h b/src/core/cgroup.h index c56979de01..841792805c 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -53,7 +53,9 @@ typedef enum CGroupDevicePolicy { typedef enum FreezerAction { FREEZER_FREEZE, + FREEZER_PARENT_FREEZE, FREEZER_THAW, + FREEZER_PARENT_THAW, _FREEZER_ACTION_MAX, _FREEZER_ACTION_INVALID = -EINVAL, @@ -433,9 +435,6 @@ bool unit_cgroup_delegate(Unit *u); int unit_get_cpuset(Unit *u, CPUSet *cpus, const char *name); int unit_cgroup_freezer_action(Unit *u, FreezerAction action); -const char* freezer_action_to_string(FreezerAction a) _const_; -FreezerAction freezer_action_from_string(const char *s) _pure_; - const char* cgroup_pressure_watch_to_string(CGroupPressureWatch a) _const_; CGroupPressureWatch cgroup_pressure_watch_from_string(const char *s) _pure_; diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 7aa44b2393..6234a638d1 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -723,7 +723,6 @@ int bus_unit_method_clean(sd_bus_message *message, void *userdata, sd_bus_error static int bus_unit_method_freezer_generic(sd_bus_message *message, void *userdata, sd_bus_error *error, FreezerAction action) { const char* perm; - int (*method)(Unit*); Unit *u = ASSERT_PTR(userdata); bool reply_no_delay = false; int r; @@ -731,13 +730,7 @@ static int bus_unit_method_freezer_generic(sd_bus_message *message, void *userda assert(message); assert(IN_SET(action, FREEZER_FREEZE, FREEZER_THAW)); - if (action == FREEZER_FREEZE) { - perm = "stop"; - method = unit_freeze; - } else { - perm = "start"; - method = unit_thaw; - } + perm = action == FREEZER_FREEZE ? "stop" : "start"; r = mac_selinux_unit_access_check(u, message, perm, error); if (r < 0) @@ -754,15 +747,17 @@ static int bus_unit_method_freezer_generic(sd_bus_message *message, void *userda if (r == 0) return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */ - r = method(u); + r = unit_freezer_action(u, action); if (r == -EOPNOTSUPP) - return sd_bus_error_setf(error, SD_BUS_ERROR_NOT_SUPPORTED, "Unit '%s' does not support freezing.", u->id); + return sd_bus_error_setf(error, SD_BUS_ERROR_NOT_SUPPORTED, "Unit does not support freeze/thaw."); if (r == -EBUSY) return sd_bus_error_set(error, BUS_ERROR_UNIT_BUSY, "Unit has a pending job."); if (r == -EHOSTDOWN) return sd_bus_error_set(error, BUS_ERROR_UNIT_INACTIVE, "Unit is inactive."); if (r == -EALREADY) - return sd_bus_error_setf(error, SD_BUS_ERROR_FAILED, "Previously requested freezer operation for unit '%s' is still in progress.", u->id); + return sd_bus_error_setf(error, SD_BUS_ERROR_FAILED, "Previously requested freezer operation for unit is still in progress."); + if (r == -ECHILD) + return sd_bus_error_setf(error, SD_BUS_ERROR_FAILED, "Unit is frozen by a parent slice."); if (r < 0) return r; if (r == 0) diff --git a/src/core/scope.c b/src/core/scope.c index e4c27da91d..3a671d5733 100644 --- a/src/core/scope.c +++ b/src/core/scope.c @@ -804,8 +804,7 @@ const UnitVTable scope_vtable = { .start = scope_start, .stop = scope_stop, - .freeze = unit_freeze_vtable_common, - .thaw = unit_thaw_vtable_common, + .freezer_action = unit_cgroup_freezer_action, .get_timeout = scope_get_timeout, diff --git a/src/core/service.c b/src/core/service.c index aacfe0d57e..087ce0d03d 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -5160,8 +5160,7 @@ const UnitVTable service_vtable = { .clean = service_clean, .can_clean = service_can_clean, - .freeze = unit_freeze_vtable_common, - .thaw = unit_thaw_vtable_common, + .freezer_action = unit_cgroup_freezer_action, .serialize = service_serialize, .deserialize_item = service_deserialize_item, diff --git a/src/core/slice.c b/src/core/slice.c index fb4f23c106..e9b2683912 100644 --- a/src/core/slice.c +++ b/src/core/slice.c @@ -347,46 +347,47 @@ static void slice_enumerate_perpetual(Manager *m) { (void) slice_make_perpetual(m, SPECIAL_SYSTEM_SLICE, NULL); } -static bool slice_freezer_action_supported_by_children(Unit *s) { +static bool slice_can_freeze(Unit *s) { Unit *member; assert(s); - UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_SLICE_OF) { - - if (member->type == UNIT_SLICE && - !slice_freezer_action_supported_by_children(member)) + UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_SLICE_OF) + if (!unit_can_freeze(member)) return false; - - if (!UNIT_VTABLE(member)->freeze) - return false; - } - return true; } static int slice_freezer_action(Unit *s, FreezerAction action) { + FreezerAction child_action; Unit *member; int r; assert(s); - assert(IN_SET(action, FREEZER_FREEZE, FREEZER_THAW)); + assert(IN_SET(action, FREEZER_FREEZE, FREEZER_PARENT_FREEZE, + FREEZER_THAW, FREEZER_PARENT_THAW)); - if (action == FREEZER_FREEZE && !slice_freezer_action_supported_by_children(s)) { + if (action == FREEZER_FREEZE && !slice_can_freeze(s)) { + /* We're intentionally only checking for FREEZER_FREEZE here and ignoring the + * _BY_PARENT variant. If we're being frozen by parent, that means someone has + * already checked if we can be frozen further up the call stack. No point to + * redo that work */ log_unit_warning(s, "Requested freezer operation is not supported by all children of the slice"); return 0; } - UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_SLICE_OF) { - if (!member->cgroup_realized) - continue; + if (action == FREEZER_FREEZE) + child_action = FREEZER_PARENT_FREEZE; + else if (action == FREEZER_THAW) + child_action = FREEZER_PARENT_THAW; + else + child_action = action; - if (action == FREEZER_FREEZE) - r = UNIT_VTABLE(member)->freeze(member); - else if (UNIT_VTABLE(member)->thaw) - r = UNIT_VTABLE(member)->thaw(member); + UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_SLICE_OF) { + if (UNIT_VTABLE(member)->freezer_action) + r = UNIT_VTABLE(member)->freezer_action(member, child_action); else - /* Thawing is requested but no corresponding method is available, ignore. */ + /* Only thawing will reach here, since freezing checks for a method in can_freeze */ r = 0; if (r < 0) return r; @@ -395,24 +396,6 @@ static int slice_freezer_action(Unit *s, FreezerAction action) { return unit_cgroup_freezer_action(s, action); } -static int slice_freeze(Unit *s) { - assert(s); - - return slice_freezer_action(s, FREEZER_FREEZE); -} - -static int slice_thaw(Unit *s) { - assert(s); - - return slice_freezer_action(s, FREEZER_THAW); -} - -static bool slice_can_freeze(Unit *s) { - assert(s); - - return slice_freezer_action_supported_by_children(s); -} - const UnitVTable slice_vtable = { .object_size = sizeof(Slice), .cgroup_context_offset = offsetof(Slice, cgroup_context), @@ -436,8 +419,7 @@ const UnitVTable slice_vtable = { .start = slice_start, .stop = slice_stop, - .freeze = slice_freeze, - .thaw = slice_thaw, + .freezer_action = slice_freezer_action, .can_freeze = slice_can_freeze, .serialize = slice_serialize, diff --git a/src/core/unit.c b/src/core/unit.c index d9e6706742..b4634e0a86 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -905,32 +905,6 @@ FreezerState unit_freezer_state(Unit *u) { return u->freezer_state; } -int unit_freezer_state_kernel(Unit *u, FreezerState *ret) { - char *values[1] = {}; - int r; - - assert(u); - - r = cg_get_keyed_attribute(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, "cgroup.events", - STRV_MAKE("frozen"), values); - if (r < 0) - return r; - - r = _FREEZER_STATE_INVALID; - - if (values[0]) { - if (streq(values[0], "0")) - r = FREEZER_RUNNING; - else if (streq(values[0], "1")) - r = FREEZER_FROZEN; - } - - free(values[0]); - *ret = r; - - return 0; -} - UnitActiveState unit_active_state(Unit *u) { assert(u); @@ -6147,19 +6121,98 @@ bool unit_can_isolate_refuse_manual(Unit *u) { return unit_can_isolate(u) && !u->refuse_manual_start; } +void unit_next_freezer_state(Unit *u, FreezerAction action, FreezerState *ret, FreezerState *ret_target) { + Unit *slice; + FreezerState curr, parent, next, tgt; + + assert(u); + assert(IN_SET(action, FREEZER_FREEZE, FREEZER_PARENT_FREEZE, + FREEZER_THAW, FREEZER_PARENT_THAW)); + assert(ret); + assert(ret_target); + + /* This function determintes the correct freezer state transitions for a unit + * given the action being requested. It returns the next state, and also the "target", + * which is either FREEZER_FROZEN or FREEZER_RUNNING, depending on what actual state we + * ultimately want to achieve. */ + + curr = u->freezer_state; + slice = UNIT_GET_SLICE(u); + if (slice) + parent = slice->freezer_state; + else + parent = FREEZER_RUNNING; + + if (action == FREEZER_FREEZE) { + /* We always "promote" a freeze initiated by parent into a normal freeze */ + if (IN_SET(curr, FREEZER_FROZEN, FREEZER_FROZEN_BY_PARENT)) + next = FREEZER_FROZEN; + else + next = FREEZER_FREEZING; + } else if (action == FREEZER_THAW) { + /* Thawing is the most complicated operation here, because we can't thaw a unit + * if its parent is frozen. So we instead "demote" a normal freeze into a freeze + * initiated by parent if the parent is frozen */ + if (IN_SET(curr, FREEZER_RUNNING, FREEZER_THAWING, FREEZER_FREEZING_BY_PARENT, FREEZER_FROZEN_BY_PARENT)) + next = curr; + else if (curr == FREEZER_FREEZING) { + if (IN_SET(parent, FREEZER_RUNNING, FREEZER_THAWING)) + next = FREEZER_THAWING; + else + next = FREEZER_FREEZING_BY_PARENT; + } else { + assert(curr == FREEZER_FROZEN); + if (IN_SET(parent, FREEZER_RUNNING, FREEZER_THAWING)) + next = FREEZER_THAWING; + else + next = FREEZER_FROZEN_BY_PARENT; + } + } else if (action == FREEZER_PARENT_FREEZE) { + /* We need to avoid accidentally demoting units frozen manually */ + if (IN_SET(curr, FREEZER_FREEZING, FREEZER_FROZEN, FREEZER_FROZEN_BY_PARENT)) + next = curr; + else + next = FREEZER_FREEZING_BY_PARENT; + } else { + assert(action == FREEZER_PARENT_THAW); + + /* We don't want to thaw units from a parent if they were frozen + * manually, so for such units this action is a no-op */ + if (IN_SET(curr, FREEZER_RUNNING, FREEZER_FREEZING, FREEZER_FROZEN)) + next = curr; + else + next = FREEZER_THAWING; + } + + tgt = freezer_state_finish(next); + if (tgt == FREEZER_FROZEN_BY_PARENT) + tgt = FREEZER_FROZEN; + assert(IN_SET(tgt, FREEZER_RUNNING, FREEZER_FROZEN)); + + *ret = next; + *ret_target = tgt; +} + bool unit_can_freeze(Unit *u) { assert(u); + if (unit_has_name(u, SPECIAL_ROOT_SLICE) || unit_has_name(u, SPECIAL_INIT_SCOPE)) + return false; + if (UNIT_VTABLE(u)->can_freeze) return UNIT_VTABLE(u)->can_freeze(u); - return UNIT_VTABLE(u)->freeze; + return UNIT_VTABLE(u)->freezer_action; } void unit_frozen(Unit *u) { assert(u); - u->freezer_state = FREEZER_FROZEN; + u->freezer_state = u->freezer_state == FREEZER_FREEZING_BY_PARENT + ? FREEZER_FROZEN_BY_PARENT + : FREEZER_FROZEN; + + log_unit_debug(u, "Unit now %s.", freezer_state_to_string(u->freezer_state)); bus_unit_send_pending_freezer_message(u, false); } @@ -6169,19 +6222,19 @@ void unit_thawed(Unit *u) { u->freezer_state = FREEZER_RUNNING; + log_unit_debug(u, "Unit thawed."); + bus_unit_send_pending_freezer_message(u, false); } -static int unit_freezer_action(Unit *u, FreezerAction action) { +int unit_freezer_action(Unit *u, FreezerAction action) { UnitActiveState s; - int (*method)(Unit*); int r; assert(u); assert(IN_SET(action, FREEZER_FREEZE, FREEZER_THAW)); - method = action == FREEZER_FREEZE ? UNIT_VTABLE(u)->freeze : UNIT_VTABLE(u)->thaw; - if (!method || !cg_freezer_supported()) + if (!cg_freezer_supported() || !unit_can_freeze(u)) return -EOPNOTSUPP; if (u->job) @@ -6194,36 +6247,21 @@ static int unit_freezer_action(Unit *u, FreezerAction action) { if (s != UNIT_ACTIVE) return -EHOSTDOWN; - if ((IN_SET(u->freezer_state, FREEZER_FREEZING, FREEZER_THAWING) && action == FREEZER_FREEZE) || - (u->freezer_state == FREEZER_THAWING && action == FREEZER_THAW)) + if (action == FREEZER_FREEZE && IN_SET(u->freezer_state, FREEZER_FREEZING, FREEZER_FREEZING_BY_PARENT)) return -EALREADY; + if (action == FREEZER_THAW && u->freezer_state == FREEZER_THAWING) + return -EALREADY; + if (action == FREEZER_THAW && IN_SET(u->freezer_state, FREEZER_FREEZING_BY_PARENT, FREEZER_FROZEN_BY_PARENT)) + return -ECHILD; - r = method(u); + r = UNIT_VTABLE(u)->freezer_action(u, action); if (r <= 0) return r; - assert(IN_SET(u->freezer_state, FREEZER_FREEZING, FREEZER_THAWING)); - + assert(IN_SET(u->freezer_state, FREEZER_FREEZING, FREEZER_FREEZING_BY_PARENT, FREEZER_THAWING)); return 1; } -int unit_freeze(Unit *u) { - return unit_freezer_action(u, FREEZER_FREEZE); -} - -int unit_thaw(Unit *u) { - return unit_freezer_action(u, FREEZER_THAW); -} - -/* Wrappers around low-level cgroup freezer operations common for service and scope units */ -int unit_freeze_vtable_common(Unit *u) { - return unit_cgroup_freezer_action(u, FREEZER_FREEZE); -} - -int unit_thaw_vtable_common(Unit *u) { - return unit_cgroup_freezer_action(u, FREEZER_THAW); -} - Condition *unit_find_failed_condition(Unit *u) { Condition *failed_trigger = NULL; bool has_succeeded_trigger = false; diff --git a/src/core/unit.h b/src/core/unit.h index fdaa3b8a8e..ae22c112b7 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -642,9 +642,9 @@ typedef struct UnitVTable { /* Clear out the various runtime/state/cache/logs/configuration data */ int (*clean)(Unit *u, ExecCleanMask m); - /* Freeze the unit */ - int (*freeze)(Unit *u); - int (*thaw)(Unit *u); + /* Freeze or thaw the unit. Returns > 0 to indicate that the request will be handled asynchronously; unit_frozen + * or unit_thawed should be called once the operation is done. Returns 0 if done successfully, or < 0 on error. */ + int (*freezer_action)(Unit *u, FreezerAction a); bool (*can_freeze)(Unit *u); /* Return which kind of data can be cleaned */ @@ -912,7 +912,6 @@ bool unit_has_name(const Unit *u, const char *name); UnitActiveState unit_active_state(Unit *u); FreezerState unit_freezer_state(Unit *u); -int unit_freezer_state_kernel(Unit *u, FreezerState *ret); const char* unit_sub_state_to_string(Unit *u); @@ -1095,15 +1094,11 @@ bool unit_can_stop_refuse_manual(Unit *u); bool unit_can_isolate_refuse_manual(Unit *u); bool unit_can_freeze(Unit *u); -int unit_freeze(Unit *u); +int unit_freezer_action(Unit *u, FreezerAction action); +void unit_next_freezer_state(Unit *u, FreezerAction a, FreezerState *ret, FreezerState *ret_tgt); void unit_frozen(Unit *u); - -int unit_thaw(Unit *u); void unit_thawed(Unit *u); -int unit_freeze_vtable_common(Unit *u); -int unit_thaw_vtable_common(Unit *u); - Condition *unit_find_failed_condition(Unit *u); int unit_arm_timer(Unit *u, sd_event_source **source, bool relative, usec_t usec, sd_event_time_handler_t handler); diff --git a/test/units/testsuite-38.sh b/test/units/testsuite-38.sh index 5fc87fca7b..0737b9c199 100755 --- a/test/units/testsuite-38.sh +++ b/test/units/testsuite-38.sh @@ -105,7 +105,14 @@ check_freezer_state() { } check_cgroup_state() { - grep -q "frozen $2" /sys/fs/cgroup/system.slice/"$1"/cgroup.events + # foo.unit -> /system.slice/foo.unit/ + # foo.slice/ -> /foo.slice/./ + # foo.slice/foo.unit -> /foo.slice/foo.unit/ + local slice unit + unit="${1##*/}" + slice="${1%"$unit"}" + slice="${slice%/}" + grep -q "frozen $2" /sys/fs/cgroup/"${slice:-system.slice}"/"${unit:-.}"/cgroup.events } testcase_dbus_api() { @@ -224,20 +231,94 @@ testcase_recursive() { echo "Test recursive freezing:" - echo -n " - freeze: " + echo -n " - freeze/thaw parent: " systemctl freeze "$slice" - check_freezer_state "${slice}" "frozen" - check_freezer_state "${unit}" "frozen" - grep -q "frozen 1" /sys/fs/cgroup/"${slice}"/cgroup.events - grep -q "frozen 1" /sys/fs/cgroup/"${slice}"/"${unit}"/cgroup.events + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen-by-parent" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl thaw "$slice" + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "running" + check_cgroup_state "$slice/" 0 + check_cgroup_state "$slice/$unit" 0 echo "[ OK ]" - echo -n " - thaw: " + echo -n " - child freeze/thaw during frozen parent: " + systemctl freeze "$slice" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen-by-parent" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl freeze "$unit" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl thaw "$unit" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen-by-parent" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 systemctl thaw "$slice" - check_freezer_state "${unit}" "running" - check_freezer_state "${slice}" "running" - grep -q "frozen 0" /sys/fs/cgroup/"${slice}"/cgroup.events - grep -q "frozen 0" /sys/fs/cgroup/"${slice}"/"${unit}"/cgroup.events + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "running" + check_cgroup_state "$slice/" 0 + check_cgroup_state "$slice/$unit" 0 + echo "[ OK ]" + + echo -n " - pre-frozen child not thawed by parent: " + systemctl freeze "$unit" + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "frozen" + check_cgroup_state "$slice/" 0 + check_cgroup_state "$slice/$unit" 1 + systemctl freeze "$slice" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl thaw "$slice" + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "frozen" + check_cgroup_state "$slice/" 0 + check_cgroup_state "$slice/$unit" 1 + echo "[ OK ]" + + echo -n " - pre-frozen child demoted and thawed by parent: " + systemctl freeze "$slice" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl thaw "$unit" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen-by-parent" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl thaw "$slice" + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "running" + check_cgroup_state "$slice/" 0 + check_cgroup_state "$slice/$unit" 0 + echo "[ OK ]" + + echo -n " - child promoted and not thawed by parent: " + systemctl freeze "$slice" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen-by-parent" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl freeze "$unit" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl thaw "$slice" + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "frozen" + check_cgroup_state "$slice/" 0 + check_cgroup_state "$slice/$unit" 1 echo "[ OK ]" systemctl stop "$unit" @@ -255,36 +336,36 @@ testcase_preserve_state() { echo "Test that freezer state is preserved when recursive freezing is initiated from outside (e.g. by manager up the tree):" echo -n " - freeze from outside: " - echo 1 >/sys/fs/cgroup/"${slice}"/cgroup.freeze + echo 1 >/sys/fs/cgroup/"$slice"/cgroup.freeze # Give kernel some time to freeze the slice sleep 1 # Our state should not be affected - check_freezer_state "${slice}" "running" - check_freezer_state "${unit}" "running" + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "running" # However actual kernel state should be frozen - grep -q "frozen 1" /sys/fs/cgroup/"${slice}"/cgroup.events - grep -q "frozen 1" /sys/fs/cgroup/"${slice}"/"${unit}"/cgroup.events + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 echo "[ OK ]" echo -n " - thaw from outside: " - echo 0 >/sys/fs/cgroup/"${slice}"/cgroup.freeze + echo 0 >/sys/fs/cgroup/"$slice"/cgroup.freeze sleep 1 - check_freezer_state "${unit}" "running" - check_freezer_state "${slice}" "running" - grep -q "frozen 0" /sys/fs/cgroup/"${slice}"/cgroup.events - grep -q "frozen 0" /sys/fs/cgroup/"${slice}"/"${unit}"/cgroup.events + check_freezer_state "$unit" "running" + check_freezer_state "$slice" "running" + check_cgroup_state "$slice/" 0 + check_cgroup_state "$slice/$unit" 0 echo "[ OK ]" echo -n " - thaw from outside while inner service is frozen: " systemctl freeze "$unit" - check_freezer_state "${unit}" "frozen" - echo 1 >/sys/fs/cgroup/"${slice}"/cgroup.freeze - echo 0 >/sys/fs/cgroup/"${slice}"/cgroup.freeze - check_freezer_state "${slice}" "running" - check_freezer_state "${unit}" "frozen" + check_freezer_state "$unit" "frozen" + echo 1 >/sys/fs/cgroup/"$slice"/cgroup.freeze + echo 0 >/sys/fs/cgroup/"$slice"/cgroup.freeze + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "frozen" echo "[ OK ]" systemctl stop "$unit" From 4cb2e6af8dccfa35b1731c1f6772fdbc3b521113 Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Tue, 23 Jan 2024 19:50:21 -0500 Subject: [PATCH 2/2] core: Fail to start/stop/reload unit if frozen Previously, unit_{start,stop,reload} would call the low-level cgroup unfreeze function whenever a unit was started, stopped, or reloaded. It did so with no error checking. This call would ultimately recurse up the cgroup tree, and unfreeze all the parent cgroups of the unit, unless an error occurred (in which case I have no idea what would happen...) After the freeze/thaw rework in a previous commit, this can no longer work. If we recursively thaw the parent cgroups of the unit, there may be sibling units marked as PARENT_FROZEN which will no longer actually have frozen parents. Fixing this is a lot more complicated than simply disallowing start/stop/reload on a frozen unit Fixes https://github.com/systemd/systemd/issues/15849 --- src/core/job.c | 9 ++++++++- src/core/job.h | 1 + src/core/unit.c | 18 ++++++++++++++---- src/shared/bus-wait-for-jobs.c | 4 ++++ test/units/testsuite-38.sh | 31 ++++++------------------------- 5 files changed, 33 insertions(+), 30 deletions(-) diff --git a/src/core/job.c b/src/core/job.c index e78c2a70db..595fbce93a 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -633,16 +633,19 @@ static const char* job_done_message_format(Unit *u, JobType t, JobResult result) [JOB_UNSUPPORTED] = "Starting of %s unsupported.", [JOB_COLLECTED] = "Unnecessary job was removed for %s.", [JOB_ONCE] = "Unit %s has been started before and cannot be started again.", + [JOB_FROZEN] = "Cannot start frozen unit %s.", }; static const char* const generic_finished_stop_job[_JOB_RESULT_MAX] = { [JOB_DONE] = "Stopped %s.", [JOB_FAILED] = "Stopped %s with error.", [JOB_TIMEOUT] = "Timed out stopping %s.", + [JOB_FROZEN] = "Cannot stop frozen unit %s.", }; static const char* const generic_finished_reload_job[_JOB_RESULT_MAX] = { [JOB_DONE] = "Reloaded %s.", [JOB_FAILED] = "Reload failed for %s.", [JOB_TIMEOUT] = "Timed out reloading %s.", + [JOB_FROZEN] = "Cannot reload frozen unit %s.", }; /* When verify-active detects the unit is inactive, report it. * Most likely a DEPEND warning from a requisiting unit will @@ -704,6 +707,7 @@ static const struct { [JOB_UNSUPPORTED] = { LOG_WARNING, ANSI_HIGHLIGHT_YELLOW, "UNSUPP" }, [JOB_COLLECTED] = { LOG_INFO, }, [JOB_ONCE] = { LOG_ERR, ANSI_HIGHLIGHT_RED, " ONCE " }, + [JOB_FROZEN] = { LOG_ERR, ANSI_HIGHLIGHT_RED, "FROZEN" }, }; static const char* job_done_mid(JobType type, JobResult result) { @@ -954,6 +958,8 @@ int job_run_and_invalidate(Job *j) { r = job_finish_and_invalidate(j, JOB_DEPENDENCY, true, false); else if (r == -ESTALE) r = job_finish_and_invalidate(j, JOB_ONCE, true, false); + else if (r == -EDEADLK) + r = job_finish_and_invalidate(j, JOB_FROZEN, true, false); else if (r < 0) r = job_finish_and_invalidate(j, JOB_FAILED, true, false); } @@ -1011,7 +1017,7 @@ int job_finish_and_invalidate(Job *j, JobResult result, bool recursive, bool alr goto finish; } - if (IN_SET(result, JOB_FAILED, JOB_INVALID)) + if (IN_SET(result, JOB_FAILED, JOB_INVALID, JOB_FROZEN)) j->manager->n_failed_jobs++; job_uninstall(j); @@ -1645,6 +1651,7 @@ static const char* const job_result_table[_JOB_RESULT_MAX] = { [JOB_UNSUPPORTED] = "unsupported", [JOB_COLLECTED] = "collected", [JOB_ONCE] = "once", + [JOB_FROZEN] = "frozen", }; DEFINE_STRING_TABLE_LOOKUP(job_result, JobResult); diff --git a/src/core/job.h b/src/core/job.h index 891d87a79c..8318b524b4 100644 --- a/src/core/job.h +++ b/src/core/job.h @@ -96,6 +96,7 @@ enum JobResult { JOB_UNSUPPORTED, /* Couldn't start a unit, because the unit type is not supported on the system */ JOB_COLLECTED, /* Job was garbage collected, since nothing needed it anymore */ JOB_ONCE, /* Unit was started before, and hence can't be started again */ + JOB_FROZEN, /* Unit is currently frozen, so we can't safely operate on it */ _JOB_RESULT_MAX, _JOB_RESULT_INVALID = -EINVAL, }; diff --git a/src/core/unit.c b/src/core/unit.c index b4634e0a86..6f61a2285e 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1956,6 +1956,10 @@ int unit_start(Unit *u, ActivationDetails *details) { return unit_start(following, details); } + /* Check to make sure the unit isn't frozen */ + if (u->freezer_state != FREEZER_RUNNING) + return -EDEADLK; + /* Check our ability to start early so that failure conditions don't cause us to enter a busy loop. */ if (UNIT_VTABLE(u)->can_start) { r = UNIT_VTABLE(u)->can_start(u); @@ -1972,7 +1976,6 @@ int unit_start(Unit *u, ActivationDetails *details) { * waits for a holdoff timer to elapse before it will start again. */ unit_add_to_dbus_queue(u); - unit_cgroup_freezer_action(u, FREEZER_THAW); if (!u->activation_details) /* Older details object wins */ u->activation_details = activation_details_ref(details); @@ -2007,6 +2010,7 @@ bool unit_can_isolate(Unit *u) { * -EBADR: This unit type does not support stopping. * -EALREADY: Unit is already stopped. * -EAGAIN: An operation is already in progress. Retry later. + * -EDEADLK: Unit is frozen */ int unit_stop(Unit *u) { UnitActiveState state; @@ -2024,11 +2028,14 @@ int unit_stop(Unit *u) { return unit_stop(following); } + /* Check to make sure the unit isn't frozen */ + if (u->freezer_state != FREEZER_RUNNING) + return -EDEADLK; + if (!UNIT_VTABLE(u)->stop) return -EBADR; unit_add_to_dbus_queue(u); - unit_cgroup_freezer_action(u, FREEZER_THAW); return UNIT_VTABLE(u)->stop(u); } @@ -2053,6 +2060,7 @@ bool unit_can_stop(Unit *u) { * -EBADR: This unit type does not support reloading. * -ENOEXEC: Unit is not started. * -EAGAIN: An operation is already in progress. Retry later. + * -EDEADLK: Unit is frozen. */ int unit_reload(Unit *u) { UnitActiveState state; @@ -2079,6 +2087,10 @@ int unit_reload(Unit *u) { return unit_reload(following); } + /* Check to make sure the unit isn't frozen */ + if (u->freezer_state != FREEZER_RUNNING) + return -EDEADLK; + unit_add_to_dbus_queue(u); if (!UNIT_VTABLE(u)->reload) { @@ -2087,8 +2099,6 @@ int unit_reload(Unit *u) { return 0; } - unit_cgroup_freezer_action(u, FREEZER_THAW); - return UNIT_VTABLE(u)->reload(u); } diff --git a/src/shared/bus-wait-for-jobs.c b/src/shared/bus-wait-for-jobs.c index 70f2043347..2930552be4 100644 --- a/src/shared/bus-wait-for-jobs.c +++ b/src/shared/bus-wait-for-jobs.c @@ -248,6 +248,8 @@ static int check_wait_response(BusWaitForJobs *d, WaitJobsFlags flags, const cha log_error("Queued job for %s was garbage collected.", d->name); else if (streq(d->result, "once")) log_error("Unit %s was started already once and can't be started again.", d->name); + else if (streq(d->result, "frozen")) + log_error("Cannot perform operation on frozen unit %s.", d->name); else if (endswith(d->name, ".service")) { /* Job result is unknown. For services, let's also try Result property. */ _cleanup_free_ char *result = NULL; @@ -276,6 +278,8 @@ static int check_wait_response(BusWaitForJobs *d, WaitJobsFlags flags, const cha return -EOPNOTSUPP; else if (streq(d->result, "once")) return -ESTALE; + else if (streq(d->result, "frozen")) + return -EDEADLK; return log_debug_errno(SYNTHETIC_ERRNO(ENOMEDIUM), "Unexpected job result '%s' for unit '%s', assuming server side newer than us.", diff --git a/test/units/testsuite-38.sh b/test/units/testsuite-38.sh index 0737b9c199..e557fa1dbf 100755 --- a/test/units/testsuite-38.sh +++ b/test/units/testsuite-38.sh @@ -149,31 +149,6 @@ testcase_dbus_api() { echo } -testcase_jobs() { - local pid_before= - local pid_after= - echo "Test that it is possible to apply jobs on frozen units:" - - systemctl start "${unit}" - dbus_freeze "${unit}" - check_freezer_state "${unit}" "frozen" - - echo -n " - restart: " - pid_before=$(systemctl show -p MainPID "${unit}" --value) - systemctl restart "${unit}" - pid_after=$(systemctl show -p MainPID "${unit}" --value) - [ "$pid_before" != "$pid_after" ] && echo "[ OK ]" - - dbus_freeze "${unit}" - check_freezer_state "${unit}" "frozen" - - echo -n " - stop: " - timeout 5s systemctl stop "${unit}" - echo "[ OK ]" - - echo -} - testcase_systemctl() { echo "Test that systemctl freeze/thaw verbs:" @@ -321,6 +296,11 @@ testcase_recursive() { check_cgroup_state "$slice/$unit" 1 echo "[ OK ]" + echo -n " - can't stop a frozen unit: " + (! systemctl -q stop "$unit" ) + echo "[ OK ]" + systemctl thaw "$unit" + systemctl stop "$unit" systemctl stop "$slice" @@ -368,6 +348,7 @@ testcase_preserve_state() { check_freezer_state "$unit" "frozen" echo "[ OK ]" + systemctl thaw "$unit" systemctl stop "$unit" systemctl stop "$slice"