vircgroup: Fix virCgroupKillRecursive() wrt nested controllers

I've encountered the following bug, but only on Gentoo with
systemd and CGroupsV2. I've started an LXC container successfully
but destroying it reported the following error:

  error: Failed to destroy domain 'amd64'
  error: internal error: failed to get cgroup backend for 'pathOfController'

Debugging showed, that CGroup hierarchy is full of surprises:

/sys/fs/cgroup/machine.slice/machine-lxc\x2d861\x2damd64.scope/
└── libvirt
    ├── dev-hugepages.mount
    ├── dev-mqueue.mount
    ├── init.scope
    ├── sys-fs-fuse-connections.mount
    ├── sys-kernel-config.mount
    ├── sys-kernel-debug.mount
    ├── sys-kernel-tracing.mount
    ├── system.slice
    │   ├── console-getty.service
    │   ├── dbus.service
    │   ├── system-getty.slice
    │   ├── system-modprobe.slice
    │   ├── systemd-journald.service
    │   ├── systemd-logind.service
    │   └── tmp.mount
    └── user.slice

For comparison, here's the same container on recent Rawhide:

/sys/fs/cgroup/machine.slice/machine-lxc\x2d13550\x2damd64.scope/
└── libvirt

Anyway, those nested directories should not be a problem, because
virCgroupKillRecursiveInternal() removes them recursively, right?
Sort of. The function really does remove nested directories, but
it assumes that every directory has the same controller as the
rest. Just take a look at virCgroupV2KillRecursive() - it gets
'Any' controller (the first one it found in ".scope") and then
passes it to virCgroupKillRecursiveInternal().

This assumption is not true though. The controllers found in
".scope" are the following:

  cpuset cpu io memory pids

while "libvirt" has fewer:

  cpuset cpu io memory

Up until now it's not problem, because of how we order
controllers internally - "cpu" is the first and thus picking
"Any" controller returns just that. But the rest of directories
has no controllers, their "cgroup.controllers" is just empty.

What fixes the bug is dropping @controller argument from
virCgroupKillRecursiveInternal() and letting each iteration work
pick its own controller.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 96280a0a4e34282e6afbd2244e83c4131ee1b43a..37dde2a5ed9afd4a9655195138e282bbaa467048 100644 (file)
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1477,6 +1477,24 @@ virCgroupHasController(virCgroup *cgroup, int controller)
 }
 
 
+static int
+virCgroupGetAnyController(virCgroup *cgroup)
+{
+    size_t i;
+
+    for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
+        if (!cgroup->backends[i])
+            continue;
+
+        return cgroup->backends[i]->getAnyController(cgroup);
+    }
+
+    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                   _("Unable to get any controller"));
+    return -1;
+}
+
+
 int
 virCgroupPathOfController(virCgroup *group,
                           unsigned int controller,
@@ -2715,11 +2733,11 @@ int
 virCgroupKillRecursiveInternal(virCgroup *group,
                                int signum,
                                GHashTable *pids,
-                               int controller,
                                const char *taskFile,
                                bool dormdir)
 {
     int rc;
+    int controller;
     bool killedAny = false;
     g_autofree char *keypath = NULL;
     g_autoptr(DIR) dp = NULL;
@@ -2728,6 +2746,9 @@ virCgroupKillRecursiveInternal(virCgroup *group,
     VIR_DEBUG("group=%p signum=%d pids=%p taskFile=%s dormdir=%d",
               group, signum, pids, taskFile, dormdir);
 
+    if ((controller = virCgroupGetAnyController(group)) < 0)
+        return -1;
+
     if (virCgroupPathOfController(group, controller, "", &keypath) < 0)
         return -1;
 
@@ -2760,7 +2781,7 @@ virCgroupKillRecursiveInternal(virCgroup *group,
             return -1;
 
         if ((rc = virCgroupKillRecursiveInternal(subgroup, signum, pids,
-                                                 controller, taskFile, true)) < 0)
+                                                 taskFile, true)) < 0)
             return -1;
         if (rc == 1)
             killedAny = true;

diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h
index 00193fb10179f0032a056e7efa0920f43d3cc7d8..caf7ed84dbe7269c1bf2b10114c2455782867602 100644 (file)
--- a/src/util/vircgrouppriv.h
+++ b/src/util/vircgrouppriv.h
@@ -135,6 +135,5 @@ int virCgroupRemoveRecursively(char *grppath);
 int virCgroupKillRecursiveInternal(virCgroup *group,
                                    int signum,
                                    GHashTable *pids,
-                                   int controller,
                                    const char *taskFile,
                                    bool dormdir);

diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 2cc7dd386a1681eaf6cceb781f09d271dc322b72..8a04bb2e4a6344cd6823817d30313b6fc72db40b 100644 (file)
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -812,12 +812,7 @@ virCgroupV1KillRecursive(virCgroup *group,
                          int signum,
                          GHashTable *pids)
 {
-    int controller = virCgroupV1GetAnyController(group);
-
-    if (controller < 0)
-        return -1;
-
-    return virCgroupKillRecursiveInternal(group, signum, pids, controller,
+    return virCgroupKillRecursiveInternal(group, signum, pids,
                                           "tasks", false);
 }
 

diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index e55521735503f466cfba3ffb9f5a77bbefaca8dd..8881d3a88a84d0b92bf542dfd48afa2871fbdf9f 100644 (file)
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -577,12 +577,7 @@ virCgroupV2KillRecursive(virCgroup *group,
                          int signum,
                          GHashTable *pids)
 {
-    int controller = virCgroupV2GetAnyController(group);
-
-    if (controller < 0)
-        return -1;
-
-    return virCgroupKillRecursiveInternal(group, signum, pids, controller,
+    return virCgroupKillRecursiveInternal(group, signum, pids,
                                           "cgroup.threads", false);
 }