[PATCH v2-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. A chardev driven from a
monitor iothread saves and deletes its socket descriptor from that
thread, and a lookup in the main loop then walks an entry that has
already been freed and crashes in find_fd().

Add a mutex and take it in the fd accessors, around the list lookup and
surgery and nothing more. Entries move when a device is realized and
when a resource is handed over, so none of this is on a hot path and an
uncontended mutex costs nothing.

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

diff --git a/migration/cpr.c b/migration/cpr.c
index f218e6a66d6..e2a76c96a2b 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) {
@@ -176,6 +203,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)) {
@@ -561,6 +590,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);

base-commit: 87f111d087cff956908ccb22daedb596b3734877
-- 
2.53.0