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

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

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. All four are listening sockets, so a libvirtd restart
reconnects them at the same moment and two threads run QLIST_INSERT_HEAD
and QLIST_REMOVE on the same list.

The core on mhw0501 shows the result. cpr_state.fds.lh_first points at a
48 byte chunk whose malloc size field carries NON_MAIN_ARENA, so an
iothread allocated it, and it now holds a glib GSourceCallback for the
listener watch. The real list is intact and unreachable behind it:
vsock0, hostnet0 x2, charchannel1, charchannel0, charserial0 and
charmonitor, headed by a freed main arena entry with id 1, which is a
guest agent channel connection entry dropped by the main loop. Both
mutators are in the wreckage.

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 commit message is deliberately free of chardev and QLIST specifics so
that the same text serves upstream. Nothing upstream reaches this state
off the main thread, so upstream gets it as hardening rather than a fix,
against a rewritten cpr.c that keeps the fds in a GHashTable.

Patch 1 is the part that also goes upstream, so it stops at the fd
accessors and the vmstate walks. Patch 2 covers the ramblock list, which
upstream does not have. Splitting them keeps patch 1 a straight mirror of
the qemu-devel posting, which should make the next rebase onto a newer
upstream cheap.

The use-after-free read of elem->fd in the cpr_delete_fd() trace, which
this series originally carried as well, is already in as b6732b19c34.

Tested on dev007 against this base: tests/qemu-iotests migrate-cpr-exec
passes all 30 subtests, which covers the chardev serial, chardev server
unix and chardev-change hot-swap cases as well as the tap and vhost ones,
and tests/qtest/migration-test --full is 86 of 86 with
QEMU_TEST_FLAKY_TESTS=1 set, so mode/reboot, mode/transfer,
mode/transfer/defer and mode/exec all run.

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

 migration/cpr.c | 86 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 71 insertions(+), 15 deletions(-)


base-commit: 87f111d087cff956908ccb22daedb596b3734877
-- 
2.53.0