cgroup: Eager realization in unit_free

unit_free(u) realizes direct parent and invalidates members mask of all
ancestors. This isn't sufficient in v1 controller hierarchies since
siblings of the freed unit may have existed only because of the removed
unit.

We cannot be lazy about the siblings because if parent(u) is also
removed, it'd migrate and rmdir cgroups for siblings(u). However,
realized masks of siblings(u) won't reflect this change.

This was a non-issue earlier, because we weren't removing cgroup
directories properly (effectively matching the stale realized mask),
removal failed because of tasks left by missing migration (see previous
commit).

Therefore, ensure realization of all units necessary to clean up after
the free'd unit.

Fixes: #14149

diff --git a/src/core/cgroup.c b/src/core/cgroup.c
index 6750bbbe1d4af895b030df8da1f826dc646e40d2..84512963a42c63202cec17b7e239bd8d1d462689 100644 (file)
--- a/src/core/cgroup.c
+++ b/src/core/cgroup.c
@@ -2105,7 +2105,7 @@ static bool unit_has_mask_enables_realized(
                 ((u->cgroup_enabled_mask | enable_mask) & CGROUP_MASK_V2) == (u->cgroup_enabled_mask & CGROUP_MASK_V2);
 }
 
-void unit_add_to_cgroup_realize_queue(Unit *u) {
+static void unit_add_to_cgroup_realize_queue(Unit *u) {
         assert(u);
 
         if (u->in_cgroup_realize_queue)
@@ -2313,10 +2313,12 @@ unsigned manager_dispatch_cgroup_realize_queue(Manager *m) {
         return n;
 }
 
-static void unit_add_siblings_to_cgroup_realize_queue(Unit *u) {
+void unit_add_siblings_to_cgroup_realize_queue(Unit *u) {
         Unit *slice;
+        Unit *p = u;
 
         /* This adds the path from the specified unit to root slice to the queue and siblings at each level.
+         * The unit itself is excluded (assuming it's handled separately).
          *
          * Propagation of realization "side-ways" (i.e. towards siblings) is relevant on cgroup-v1 where
          * scheduling becomes very weird if two units that own processes reside in the same slice, but one is
@@ -2326,7 +2328,7 @@ static void unit_add_siblings_to_cgroup_realize_queue(Unit *u) {
          * at all are always realized in *all* their hierarchies, and it is sufficient for a unit's sibling
          * to be realized for the unit itself to be realized too. */
 
-        while ((slice = UNIT_DEREF(u->slice))) {
+        while ((slice = UNIT_DEREF(p->slice))) {
                 Iterator i;
                 Unit *m;
                 void *v;
@@ -2335,6 +2337,9 @@ static void unit_add_siblings_to_cgroup_realize_queue(Unit *u) {
                         /* Skip units that have a dependency on the slice but aren't actually in it. */
                         if (UNIT_DEREF(m->slice) != slice)
                                 continue;
+                        /* The origin must be handled separately by caller */
+                        if (m == u)
+                                continue;
 
                         /* No point in doing cgroup application for units without active processes. */
                         if (UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(m)))
@@ -2355,11 +2360,12 @@ static void unit_add_siblings_to_cgroup_realize_queue(Unit *u) {
                         unit_add_to_cgroup_realize_queue(m);
                 }
 
-                u = slice;
+                p = slice;
         }
 
         /* Root slice comes last */
-        unit_add_to_cgroup_realize_queue(u);
+        if (p != u)
+                unit_add_to_cgroup_realize_queue(p);
 }
 
 int unit_realize_cgroup(Unit *u) {

diff --git a/src/core/cgroup.h b/src/core/cgroup.h
index 52d028e740ff38e7c29a07be98f32c1e8c15adca..d6e170cdc6c4daf138e3d99cd1c7642961c3342a 100644 (file)
--- a/src/core/cgroup.h
+++ b/src/core/cgroup.h
@@ -212,7 +212,7 @@ CGroupMask unit_get_enable_mask(Unit *u);
 
 void unit_invalidate_cgroup_members_masks(Unit *u);
 
-void unit_add_to_cgroup_realize_queue(Unit *u);
+void unit_add_siblings_to_cgroup_realize_queue(Unit *u);
 
 const char *unit_get_realized_cgroup_path(Unit *u, CGroupMask mask);
 char *unit_default_cgroup_path(const Unit *u);

diff --git a/src/core/unit.c b/src/core/unit.c
index 2c09def06f3b228450cea414b582287422175fe5..c16e7562d88df3d1f873da3ae8b038f2523364c0 100644 (file)
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -620,14 +620,6 @@ void unit_free(Unit *u) {
         if (!u)
                 return;
 
-        if (UNIT_ISSET(u->slice)) {
-                /* A unit is being dropped from the tree, make sure our parent slice recalculates the member mask */
-                unit_invalidate_cgroup_members_masks(UNIT_DEREF(u->slice));
-
-                /* And make sure the parent is realized again, updating cgroup memberships */
-                unit_add_to_cgroup_realize_queue(UNIT_DEREF(u->slice));
-        }
-
         u->transient_file = safe_fclose(u->transient_file);
 
         if (!MANAGER_IS_RELOADING(u->manager))
@@ -669,6 +661,14 @@ void unit_free(Unit *u) {
         for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
                 bidi_set_free(u, u->dependencies[d]);
 
+        /* A unit is being dropped from the tree, make sure our siblings and ancestor slices are realized
+         * properly. Do this after we detach the unit from slice tree in order to eliminate its effect on
+         * controller masks. */
+        if (UNIT_ISSET(u->slice)) {
+                unit_invalidate_cgroup_members_masks(UNIT_DEREF(u->slice));
+                unit_add_siblings_to_cgroup_realize_queue(u);
+        }
+
         if (u->on_console)
                 manager_unref_console(u->manager);