[PATCH hci-8.0 1/2] migration/cpr: lock the saved fds #VSTOR-145529

["Denis V. Lunev" <den@openvz.org>]

From: Denis V. Lunev <den@openvz.org>

The cpr state is a process wide registry of the resources that have to
survive an exec. Entries are added and dropped from realize and teardown
paths all over the tree, and a caller has no way to tell which thread the
next caller will run in.

The state carries no serialization of its own. It is safe today only
because its callers happen to hold the BQL, and that is a property of
where those callers live rather than anything the interface promises or
checks. A caller that is correct within its own subsystem can therefore
break this one from a distance: two that overlap corrupt the registry,
and the next lookup walks an entry that has already been freed.

Give the state a lock of its own instead of relying on that. Entries
move when a device is realized, when a resource is handed over, and when
a migration starts, so none of this is on a hot path and an uncontended
mutex costs nothing.

Take the lock in every fd accessor and across the save and load walks.
Holding it over the walk callbacks is safe because none of them comes
back through this interface. State that is not reached through these
accessors, such as the VFIO device list that hw/vfio manipulates
directly, stays outside it.

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 migration/cpr.c | 60 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 50 insertions(+), 10 deletions(-)

diff --git a/migration/cpr.c b/migration/cpr.c
index f218e6a66d6..78cf430e386 100644
--- a/migration/cpr.c
+++ b/migration/cpr.c
@@ -9,6 +9,7 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
+#include "qemu/lockable.h"
 #include "hw/vfio/vfio-device.h"
 #include "migration/cpr.h"
 #include "migration/misc.h"
@@ -33,6 +34,14 @@
 
 CprState cpr_state;
 
+/* Callers are spread across the tree and need not share a thread. */
+static QemuMutex cpr_state_lock;
+
+static void __attribute__((constructor)) cpr_state_lock_init(void)
+{
+    qemu_mutex_init(&cpr_state_lock);
+}
+
 /****************************************************************************/
 
 typedef struct CprFd {
@@ -107,10 +116,22 @@ static CprFd *find_fd(CprFdList *head, const char *name, int id)
     return NULL;
 }
 
-void cpr_delete_fd(const char *name, int id)
+static int do_cpr_find_fd(const char *name, int id)
 {
     CprFd *elem = find_fd(&cpr_state.fds, name, id);
-    int fd = elem ? elem->fd : -1;
+
+    return elem ? elem->fd : -1;
+}
+
+void cpr_delete_fd(const char *name, int id)
+{
+    CprFd *elem;
+    int fd;
+
+    QEMU_LOCK_GUARD(&cpr_state_lock);
+
+    elem = find_fd(&cpr_state.fds, name, id);
+    fd = elem ? elem->fd : -1;
 
     if (elem) {
         QLIST_REMOVE(elem, next);
@@ -125,6 +146,8 @@ void cpr_delete_fd_all(const char *name)
 {
     CprFd *elem, *next_elem;
 
+    QEMU_LOCK_GUARD(&cpr_state_lock);
+
     QLIST_FOREACH_SAFE(elem, &cpr_state.fds, next, next_elem) {
         if (!strcmp(elem->name, name)) {
             QLIST_REMOVE(elem, next);
@@ -136,18 +159,22 @@ void cpr_delete_fd_all(const char *name)
 
 int cpr_find_fd(const char *name, int id)
 {
-    CprFd *elem = find_fd(&cpr_state.fds, name, id);
-    int fd = elem ? elem->fd : -1;
+    int fd;
+
+    QEMU_LOCK_GUARD(&cpr_state_lock);
 
+    fd = do_cpr_find_fd(name, id);
     trace_cpr_find_fd(name, id, fd);
     return fd;
 }
 
 void cpr_save_fd(const char *name, int id, int fd)
 {
-    CprFd *elem = find_fd(&cpr_state.fds, name, id);
-    int old_fd = elem ? elem->fd : -1;
+    int old_fd;
+
+    QEMU_LOCK_GUARD(&cpr_state_lock);
 
+    old_fd = do_cpr_find_fd(name, id);
     if (old_fd < 0) {
         do_cpr_save_fd(name, id, fd);
     } else if (old_fd != fd) {
@@ -161,12 +188,17 @@ void cpr_save_fd(const char *name, int id, int fd)
 int cpr_open_fd(const char *path, int flags, const char *name, int id,
                 Error **errp)
 {
-    int fd = cpr_find_fd(name, id);
+    int fd;
+
+    QEMU_LOCK_GUARD(&cpr_state_lock);
+
+    fd = do_cpr_find_fd(name, id);
+    trace_cpr_find_fd(name, id, fd);
 
     if (fd < 0) {
         fd = qemu_open(path, flags, errp);
         if (fd >= 0) {
-            cpr_save_fd(name, id, fd);
+            do_cpr_save_fd(name, id, fd);
         }
     }
     return fd;
@@ -176,6 +208,8 @@ bool cpr_walk_fd(cpr_walk_fd_cb cb)
 {
     CprFd *elem;
 
+    QEMU_LOCK_GUARD(&cpr_state_lock);
+
     QLIST_FOREACH(elem, &cpr_state.fds, next) {
         g_assert(elem->fd >= 0);
         if (!cb(elem->fd)) {
@@ -527,7 +561,9 @@ bool cpr_state_save(MigrationChannel *channel, Error **errp)
     qemu_put_be32(f, QEMU_CPR_FILE_MAGIC);
     qemu_put_be32(f, QEMU_CPR_FILE_VERSION);
 
-    ret = vmstate_save_state(f, &vmstate_cpr_state, &cpr_state, 0, errp);
+    WITH_QEMU_LOCK_GUARD(&cpr_state_lock) {
+        ret = vmstate_save_state(f, &vmstate_cpr_state, &cpr_state, 0, errp);
+    }
     if (ret) {
         qemu_fclose(f);
         return false;
@@ -561,6 +597,8 @@ static void cpr_prune_closed_fds(void)
 {
     CprFd *elem, *next;
 
+    QEMU_LOCK_GUARD(&cpr_state_lock);
+
     QLIST_FOREACH_SAFE(elem, &cpr_state.fds, next, next) {
         if (fcntl(elem->fd, F_GETFD) < 0 && errno == EBADF) {
             trace_cpr_prune_closed_fd(elem->name, elem->id, elem->fd);
@@ -611,7 +649,9 @@ int cpr_state_load(MigrationChannel *channel, Error **errp)
         return -ENOTSUP;
     }
 
-    ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);
+    WITH_QEMU_LOCK_GUARD(&cpr_state_lock) {
+        ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);
+    }
     if (ret) {
         qemu_fclose(f);
         return ret;
-- 
2.53.0