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

[Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>]

  1. Major: lock ordering is created but nowhere documented. This patch
already establishes two orders: ram_list.mutex → cpr_state_lock via
qemu_ram_free() calling cpr_delete_fd() at system/physmem.c:2409, and
chr_write_lock → cpr_state_lock via tcp_chr_disconnect_locked() →
tcp_chr_free_connection() at chardev/char-socket.c:361. The ramlist
order is only mentioned in patch 2, and the chardev order is not
mentioned at all. Put a comment at the lock definition stating it is a
leaf lock and listing what may be held on entry.

  2. Minor: error_report() under the lock inverts the chardev order.
qemu_file_put_fd() calls error_report_err() on failure while
cpr_state_save() holds the lock. From an HMP migrate, error_vprintf()
routes to monitor_vprintf(), which takes the monitor chardev's
chr_write_lock. That is the reverse of order in point 1. No deadlock
today since HMP chardevs live on the main thread, but it is exactly the
kind of remote breakage the commit message argues against. The
error_report() calls in cpr_save_fd() and cpr_save_ramblock() abort
right after, so they are moot. Either note the rule in the comment or
move error reporting out of the guarded region.

  3. Minor: the commit message overstates what stays outside the lock.
vmstate_save_state() and vmstate_load_state() run under the lock and
read or populate vfio_devices and hotplug_cpus. cpr_cpu_hotplug_needed()
fills hotplug_cpus under the lock, while cpr_load_cpu_hotplug_state()
and cpr_free_cpu_hotplug_state() walk and free it unlocked from vl.c.
Both lists are BQL-only in practice. Document per list in
include/migration/cpr.h next to CprState which ones the mutex covers.

  4. Minor: cpr_open_fd() holds the mutex across qemu_open(). A blocking
open of /dev/vfio/N or /dev/iommu stalls every other accessor, including
an iothread chardev disconnect. The fdset path also adds cpr_state_lock
→ mon_fdsets_lock. The atomic find-then-save is a real improvement over
upstream's two separate calls, so keeping it is defensible, but say so
in the commit message and add the ordering to the comment.

  5. Nit: check-then-act across two acquisitions remains. Callers such
as tcp_chr_free_connection() do cpr_find_fd() == fd then
cpr_delete_fd(). The lock protects the list, not the returned fd, and
another thread can replace the entry between the two calls. Harmless for
the existing callers. A cpr_delete_fd_if(name, id, fd) helper would make
the pattern atomic. Optional.

On 9/16/26 5:38 PM, Denis V. Lunev wrote:
> 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;