[libvirt] [PATCH] cgroup: be robust against cgroup movement races

[Eric Blake]

https://bugzilla.redhat.com/show_bug.cgi?id=965169 documents a
problem starting domains when cgroups are enabled; I was able
to reliably reproduce the race about 5% of the time when I added
hooks to domain startup by 3 seconds (as that seemed to be about
the length of time that qemu created and then closed a temporary
thread, probably related to aio handling of initially opening
a disk image).

There are some inherent TOCTTOU races when moving tasks between
kernel cgroups, precisely because threads can be created or
completed in the window between when we read a thread id from the
source and when we write to the destination.  As the goal of
virCgroupMoveTask is merely to move ALL tasks into the new
cgroup, it is sufficient to iterate until no more threads are
being created in the old group, and ignoring any threads that
die before we can move them.

It would be nicer to start the threads in the right cgroup to
begin with, but by default, all child threads are created in
the same cgroup as their parent, and we don't want vcpu child
threads in the emulator cgroup, so I don't see any good way
of avoiding the move.  It would also be nice if the kernel were
to implement something like rename() as a way to atomically move
a group of threads from one cgroup to another, instead of forcing
a window where we have to read and parse the source, then format
and write back into the destination.

* src/util/vircgroup.c (virCgroupAddTaskStrController): Ignore
ESRCH, because a thread ended between read and write attempts.
(virCgroupMoveTask): Loop until all threads have moved.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 src/util/vircgroup.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 07ea2c3..6d44694 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1037,7 +1037,11 @@ static int virCgroupAddTaskStrController(virCgroupPtr group,
             goto cleanup;

         rc = virCgroupAddTaskController(group, p, controller);
-        if (rc != 0)
+        /* A thread that exits between when we first read the source
+         * tasks and now is not fatal.  */
+        if (rc == -ESRCH)
+            rc = 0;
+        else if (rc != 0)
             goto cleanup;

         next = strchr(cur, '\n');
@@ -1074,15 +1078,23 @@ int virCgroupMoveTask(virCgroupPtr src_group, virCgroupPtr dest_group)
             !dest_group->controllers[i].mountPoint)
             continue;

-        rc = virCgroupGetValueStr(src_group, i, "tasks", &content);
-        if (rc != 0)
-            return rc;
+        /* New threads are created in the same group as their parent;
+         * but if a thread is created after we first read we aren't
+         * aware that it needs to move.  Therefore, we must iterate
+         * until content is empty.  */
+        while (1) {
+            rc = virCgroupGetValueStr(src_group, i, "tasks", &content);
+            if (rc != 0)
+                return rc;
+            if (!*content)
+                break;

-        rc = virCgroupAddTaskStrController(dest_group, content, i);
-        if (rc != 0)
-            goto cleanup;
+            rc = virCgroupAddTaskStrController(dest_group, content, i);
+            if (rc != 0)
+                goto cleanup;

-        VIR_FREE(content);
+            VIR_FREE(content);
+        }
     }

 cleanup:
-- 
1.8.1.4