[PATCH v2-hci-8.0 0/2] migration/cpr: lock the cpr state #VSTOR-145529

[PATCH v2-hci-8.0 0/2] migration/cpr: lock the cpr state #VSTOR-145529

[Denis V. Lunev]

A guest died with SIGSEGV in find_fd() after a routine libvirtd restart.
The fd registry in cpr_state has no lock, and since we started saving
chardev connection fds it is reached from more than one thread.

A socket chardev saves and drops its connection fd in
tcp_chr_new_client() and tcp_chr_free_connection(), which run in the
chardev's GMainContext. For libvirt's monitor that context belongs to the
QMP iothread, while the guest agent channels and the serial pty stay on
the main loop. A libvirtd restart reconnects all of them at once, so two
threads run QLIST_INSERT_HEAD and QLIST_REMOVE on one list. The core on
mhw0501 shows the head left on an entry the iothread allocated and freed,
with the real list intact and unreachable behind it.

Taking the BQL in the chardev hooks is not an option. The chardev layer
never takes it, monitor_cleanup() stops the QMP iothread while holding
it, and the iothread exists precisely so out-of-band commands keep
working when the main thread will not release it. The lock belongs to the
registry.

The body is worded free of chardev and QLIST specifics on purpose: the
same text serves upstream, where cpr.c has been rewritten around a
GHashTable and nothing reaches the state off the main thread. Patch 1 is
that shared part, so it stops at the fd accessors. Patch 2 covers the
ramblock list, which upstream does not have.

v1 -> v2, all of it narrowing the lock to a leaf over the list alone:

- out of the vmstate walks. The save walk runs the subsection callbacks
  and hands each fd to the peer with a blocking write, and a load hook
  that saved an fd would deadlock against itself. The load side needs no
  lock anyway, running before qemu_create_early_backends().
- out of cpr_open_fd(), which held it across qemu_open().
- patch 1's commit message trimmed to match.

So cpr_state_save() now walks the fd list unlocked and can still race a
chardev accept. Closing that needs a copy of the list taken under the
lock, which first needs cpr_cpu_hotplug_needed() to use its opaque
instead of the global cpr_state.

Tested on dev-cl04 against this base: migrate-cpr-exec, migration-test
--full, check-unit and the raw iotests all pass. The qcow2 iotests leave
vz-kernel-cbt and vz-mt-vendor, which fail the same way on the parent
commit.

Denis V. Lunev (2):
  migration/cpr: lock the saved fds #VSTOR-145529
  migration/cpr: lock the ramblocks #VSTOR-145529

 migration/cpr.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 58 insertions(+), 11 deletions(-)


base-commit: 87f111d087cff956908ccb22daedb596b3734877
-- 
2.53.0

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

[Denis V. Lunev]

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

[PATCH v2-hci-8.0 2/2] migration/cpr: lock the ramblocks #VSTOR-145529

[Denis V. Lunev]

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

The ramblock entries live in the same registry as the descriptors and
are reached the same way, from RAM block setup and teardown, so they
need the same protection.

Take the lock in their accessors too. qemu_ram_free() already holds the
ramlist mutex when it calls in here, and nothing takes the two in the
other order.

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

diff --git a/migration/cpr.c b/migration/cpr.c
index e2a76c96a2b..a149e790c10 100644
--- a/migration/cpr.c
+++ b/migration/cpr.c
@@ -239,8 +239,13 @@ static CprRamBlock *find_ramblock(CprRamBlockList *head, const char *name)
 
 void cpr_save_ramblock(const char *name, cpr_ramblock_t ram)
 {
-    CprRamBlock *elem = find_ramblock(&cpr_state.ramblocks, name);
-    cpr_ramblock_t old_ram = elem ? elem->ram : (cpr_ramblock_t) {0, 0};
+    CprRamBlock *elem;
+    cpr_ramblock_t old_ram;
+
+    QEMU_LOCK_GUARD(&cpr_state_lock);
+
+    elem = find_ramblock(&cpr_state.ramblocks, name);
+    old_ram = elem ? elem->ram : (cpr_ramblock_t) {0, 0};
 
     if (old_ram.addr == 0) {
         do_cpr_save_ramblock(name, ram);
@@ -255,7 +260,11 @@ void cpr_save_ramblock(const char *name, cpr_ramblock_t ram)
 
 void cpr_delete_ramblock(const char *name)
 {
-    CprRamBlock *elem = find_ramblock(&cpr_state.ramblocks, name);
+    CprRamBlock *elem;
+
+    QEMU_LOCK_GUARD(&cpr_state_lock);
+
+    elem = find_ramblock(&cpr_state.ramblocks, name);
 
     if (elem) {
         QLIST_REMOVE(elem, next);
@@ -268,8 +277,13 @@ void cpr_delete_ramblock(const char *name)
 
 cpr_ramblock_t cpr_find_ramblock(const char *name)
 {
-    CprRamBlock *elem = find_ramblock(&cpr_state.ramblocks, name);
-    cpr_ramblock_t ram = elem ? elem->ram : (cpr_ramblock_t) {0, 0};
+    CprRamBlock *elem;
+    cpr_ramblock_t ram;
+
+    QEMU_LOCK_GUARD(&cpr_state_lock);
+
+    elem = find_ramblock(&cpr_state.ramblocks, name);
+    ram = elem ? elem->ram : (cpr_ramblock_t) {0, 0};
 
     trace_cpr_find_ramblock(name, (void *) ram.addr, ram.length);
     return ram;
@@ -279,6 +293,8 @@ bool cpr_walk_ramblock(cpr_walk_ramblock_cb cb)
 {
     CprRamBlock *elem;
 
+    QEMU_LOCK_GUARD(&cpr_state_lock);
+
     QLIST_FOREACH(elem, &cpr_state.ramblocks, next) {
         g_assert(elem->ram.addr && elem->ram.length);
         if (!cb(elem->ram, elem->name)) {
-- 
2.53.0