[PULL 00/10] Migration 20240317 patches

[PULL 00/10] Migration 20240317 patches

[peterx]

From: Peter Xu <peterx@redhat.com>

The following changes since commit 35ac6831d98e18e2c78c85c93e3a6ca1f1ae3e58:

  Merge tag 'net-pull-request' of https://github.com/jasowang/qemu into staging (2024-03-12 13:42:57 +0000)

are available in the Git repository at:

  https://gitlab.com/peterx/qemu.git tags/migration-20240317-pull-request

for you to fetch changes up to 9adfb308c1513562d6acec02aa780c5ef9b0193d:

  migration/multifd: Duplicate the fd for the outgoing_args (2024-03-15 11:26:33 -0400)

----------------------------------------------------------------
Migration pull for 9.0-rc0

- Nicholas/Phil's fix on migration corruption / inconsistent for tcg
- Cedric's fix on block migration over n_sectors==0
- Steve's CPR reboot documentation page
- Fabiano's misc fixes on mapped-ram (IOC leak, dup() errors, fd checks, fd
  use race, etc.)

----------------------------------------------------------------

Cédric Le Goater (1):
  migration: Skip only empty block devices

Fabiano Rosas (5):
  io: Introduce qio_channel_file_new_dupfd
  migration: Fix error handling after dup in file migration
  migration: Fix iocs leaks during file and fd migration
  migration/multifd: Ensure we're not given a socket for file migration
  migration/multifd: Duplicate the fd for the outgoing_args

Nicholas Piggin (2):
  physmem: Factor cpu_physical_memory_dirty_bits_cleared() out
  physmem: Fix migration dirty bitmap coherency with TCG memory access

Philippe Mathieu-Daudé (1):
  physmem: Expose tlb_reset_dirty_range_all()

Steve Sistare (1):
  migration: cpr-reboot documentation

 docs/devel/migration/CPR.rst      | 147 ++++++++++++++++++++++++++++++
 docs/devel/migration/features.rst |   1 +
 include/exec/exec-all.h           |   1 +
 include/exec/ram_addr.h           |  12 +++
 include/io/channel-file.h         |  18 ++++
 migration/file.h                  |   1 +
 io/channel-file.c                 |  12 +++
 migration/block.c                 |   5 +-
 migration/fd.c                    |  51 ++++++-----
 migration/file.c                  |  75 +++++++++------
 migration/migration.c             |   6 +-
 system/physmem.c                  |  10 +-
 12 files changed, 279 insertions(+), 60 deletions(-)
 create mode 100644 docs/devel/migration/CPR.rst

-- 
2.44.0

[PULL 01/10] io: Introduce qio_channel_file_new_dupfd

[peterx]

From: Fabiano Rosas <farosas@suse.de>

Add a new helper function for creating a QIOChannelFile channel with a
duplicated file descriptor. This saves the calling code from having to
do error checking on the dup() call.

Suggested-by: "Daniel P. Berrangé" <berrange@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: "Daniel P. Berrangé" <berrange@redhat.com>
Link: https://lore.kernel.org/r/20240311233335.17299-2-farosas@suse.de
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/io/channel-file.h | 18 ++++++++++++++++++
 io/channel-file.c         | 12 ++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/include/io/channel-file.h b/include/io/channel-file.h
index 50e8eb1138..d373a4e44d 100644
--- a/include/io/channel-file.h
+++ b/include/io/channel-file.h
@@ -68,6 +68,24 @@ struct QIOChannelFile {
 QIOChannelFile *
 qio_channel_file_new_fd(int fd);
 
+/**
+ * qio_channel_file_new_dupfd:
+ * @fd: the file descriptor
+ * @errp: pointer to initialized error object
+ *
+ * Create a new IO channel object for a file represented by the @fd
+ * parameter. Like qio_channel_file_new_fd(), but the @fd is first
+ * duplicated with dup().
+ *
+ * The channel will own the duplicated file descriptor and will take
+ * responsibility for closing it, the original FD is owned by the
+ * caller.
+ *
+ * Returns: the new channel object
+ */
+QIOChannelFile *
+qio_channel_file_new_dupfd(int fd, Error **errp);
+
 /**
  * qio_channel_file_new_path:
  * @path: the file path
diff --git a/io/channel-file.c b/io/channel-file.c
index a6ad7770c6..6436cfb6ae 100644
--- a/io/channel-file.c
+++ b/io/channel-file.c
@@ -45,6 +45,18 @@ qio_channel_file_new_fd(int fd)
     return ioc;
 }
 
+QIOChannelFile *
+qio_channel_file_new_dupfd(int fd, Error **errp)
+{
+    int newfd = dup(fd);
+
+    if (newfd < 0) {
+        error_setg_errno(errp, errno, "Could not dup FD %d", fd);
+        return NULL;
+    }
+
+    return qio_channel_file_new_fd(newfd);
+}
 
 QIOChannelFile *
 qio_channel_file_new_path(const char *path,
-- 
2.44.0

[PULL 02/10] migration: Fix error handling after dup in file migration

[peterx]

From: Fabiano Rosas <farosas@suse.de>

The file migration code was allowing a possible -1 from a failed call
to dup() to propagate into the new QIOFileChannel::fd before checking
for validity. Coverity doesn't like that, possibly due to the the
lseek(-1, ...) call that would ensue before returning from the channel
creation routine.

Use the newly introduced qio_channel_file_dupfd() to properly check
the return of dup() before proceeding.

Fixes: CID 1539961
Fixes: CID 1539965
Fixes: CID 1539960
Fixes: 2dd7ee7a51 ("migration/multifd: Add incoming QIOChannelFile support")
Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI")
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: "Daniel P. Berrangé" <berrange@redhat.com>
Link: https://lore.kernel.org/r/20240311233335.17299-3-farosas@suse.de
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/fd.c   |  9 ++++-----
 migration/file.c | 14 +++++++-------
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/migration/fd.c b/migration/fd.c
index d4ae72d132..4e2a63a73d 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -80,6 +80,7 @@ static gboolean fd_accept_incoming_migration(QIOChannel *ioc,
 void fd_start_incoming_migration(const char *fdname, Error **errp)
 {
     QIOChannel *ioc;
+    QIOChannelFile *fioc;
     int fd = monitor_fd_param(monitor_cur(), fdname, errp);
     if (fd == -1) {
         return;
@@ -103,15 +104,13 @@ void fd_start_incoming_migration(const char *fdname, Error **errp)
         int channels = migrate_multifd_channels();
 
         while (channels--) {
-            ioc = QIO_CHANNEL(qio_channel_file_new_fd(dup(fd)));
-
-            if (QIO_CHANNEL_FILE(ioc)->fd == -1) {
-                error_setg(errp, "Failed to duplicate fd %d", fd);
+            fioc = qio_channel_file_new_dupfd(fd, errp);
+            if (!fioc) {
                 return;
             }
 
             qio_channel_set_name(ioc, "migration-fd-incoming");
-            qio_channel_add_watch_full(ioc, G_IO_IN,
+            qio_channel_add_watch_full(QIO_CHANNEL(fioc), G_IO_IN,
                                        fd_accept_incoming_migration,
                                        NULL, NULL,
                                        g_main_context_get_thread_default());
diff --git a/migration/file.c b/migration/file.c
index b0b963e0ce..e56c5eb0a5 100644
--- a/migration/file.c
+++ b/migration/file.c
@@ -58,12 +58,13 @@ bool file_send_channel_create(gpointer opaque, Error **errp)
     int fd = fd_args_get_fd();
 
     if (fd && fd != -1) {
-        ioc = qio_channel_file_new_fd(dup(fd));
+        ioc = qio_channel_file_new_dupfd(fd, errp);
     } else {
         ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp);
-        if (!ioc) {
-            goto out;
-        }
+    }
+
+    if (!ioc) {
+        goto out;
     }
 
     multifd_channel_connect(opaque, QIO_CHANNEL(ioc));
@@ -147,10 +148,9 @@ void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp)
                                    NULL, NULL,
                                    g_main_context_get_thread_default());
 
-        fioc = qio_channel_file_new_fd(dup(fioc->fd));
+        fioc = qio_channel_file_new_dupfd(fioc->fd, errp);
 
-        if (!fioc || fioc->fd == -1) {
-            error_setg(errp, "Error creating migration incoming channel");
+        if (!fioc) {
             break;
         }
     } while (++i < channels);
-- 
2.44.0

[PULL 03/10] physmem: Expose tlb_reset_dirty_range_all()

[peterx]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

In order to call tlb_reset_dirty_range_all() outside of
system/physmem.c, expose its prototype.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Link: https://lore.kernel.org/r/20240312201458.79532-2-philmd@linaro.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/exec/exec-all.h | 1 +
 system/physmem.c        | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index ce36bb10d4..3e53501691 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -655,6 +655,7 @@ static inline void mmap_unlock(void) {}
 
 void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length);
 void tlb_set_dirty(CPUState *cpu, vaddr addr);
+void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length);
 
 MemoryRegionSection *
 address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
diff --git a/system/physmem.c b/system/physmem.c
index 6cfb7a80ab..5441480ff0 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -819,7 +819,7 @@ found:
     return block;
 }
 
-static void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
+void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
 {
     CPUState *cpu;
     ram_addr_t start1;
-- 
2.44.0

[PULL 04/10] physmem: Factor cpu_physical_memory_dirty_bits_cleared() out

[peterx]

From: Nicholas Piggin <npiggin@gmail.com>

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Message-ID: <20240219061731.232570-1-npiggin@gmail.com>
[PMD: Split patch in 2: part 1/2]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Link: https://lore.kernel.org/r/20240312201458.79532-3-philmd@linaro.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/exec/ram_addr.h | 9 +++++++++
 system/physmem.c        | 8 +++-----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 90676093f5..b060ea9176 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -25,6 +25,7 @@
 #include "sysemu/tcg.h"
 #include "exec/ramlist.h"
 #include "exec/ramblock.h"
+#include "exec/exec-all.h"
 
 extern uint64_t total_dirty_pages;
 
@@ -443,6 +444,14 @@ uint64_t cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
 }
 #endif /* not _WIN32 */
 
+static inline void cpu_physical_memory_dirty_bits_cleared(ram_addr_t start,
+                                                          ram_addr_t length)
+{
+    if (tcg_enabled()) {
+        tlb_reset_dirty_range_all(start, length);
+    }
+
+}
 bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
                                               ram_addr_t length,
                                               unsigned client);
diff --git a/system/physmem.c b/system/physmem.c
index 5441480ff0..a4fe3d2bf8 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -881,8 +881,8 @@ bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
         memory_region_clear_dirty_bitmap(ramblock->mr, mr_offset, mr_size);
     }
 
-    if (dirty && tcg_enabled()) {
-        tlb_reset_dirty_range_all(start, length);
+    if (dirty) {
+        cpu_physical_memory_dirty_bits_cleared(start, length);
     }
 
     return dirty;
@@ -929,9 +929,7 @@ DirtyBitmapSnapshot *cpu_physical_memory_snapshot_and_clear_dirty
         }
     }
 
-    if (tcg_enabled()) {
-        tlb_reset_dirty_range_all(start, length);
-    }
+    cpu_physical_memory_dirty_bits_cleared(start, length);
 
     memory_region_clear_dirty_bitmap(mr, offset, length);
 
-- 
2.44.0

[PULL 05/10] physmem: Fix migration dirty bitmap coherency with TCG memory access

[peterx]

From: Nicholas Piggin <npiggin@gmail.com>

The fastpath in cpu_physical_memory_sync_dirty_bitmap() to test large
aligned ranges forgot to bring the TCG TLB up to date after clearing
some of the dirty memory bitmap bits. This can result in stores though
the TCG TLB not setting the dirty memory bitmap and ultimately causes
memory corruption / lost updates during migration from a TCG host.

Fix this by calling cpu_physical_memory_dirty_bits_cleared() when
dirty bits have been cleared.

Fixes: aa8dc044772 ("migration: synchronize memory bitmap 64bits at a time")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Message-ID: <20240219061731.232570-1-npiggin@gmail.com>
[PMD: Split patch in 2: part 2/2, slightly adapt description]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Link: https://lore.kernel.org/r/20240312201458.79532-4-philmd@linaro.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/exec/ram_addr.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index b060ea9176..de45ba7bc9 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -513,6 +513,9 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
                 idx++;
             }
         }
+        if (num_dirty) {
+            cpu_physical_memory_dirty_bits_cleared(start, length);
+        }
 
         if (rb->clear_bmap) {
             /*
-- 
2.44.0

[PULL 06/10] migration: Skip only empty block devices

[peterx]

From: Cédric Le Goater <clg@redhat.com>

The block .save_setup() handler calls a helper routine
init_blk_migration() which builds a list of block devices to take into
account for migration. When one device is found to be empty (sectors
== 0), the loop exits and all the remaining devices are ignored. This
is a regression introduced when bdrv_iterate() was removed.

Change that by skipping only empty devices.

Cc: Markus Armbruster <armbru@redhat.com>
Cc: qemu-stable <qemu-stable@nongnu.org>
Suggested-by: Kevin Wolf <kwolf@redhat.com>
Fixes: fea68bb6e9fa ("block: Eliminate bdrv_iterate(), use bdrv_next()")
Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Link: https://lore.kernel.org/r/20240312120431.550054-1-clg@redhat.com
[peterx: fix "Suggested-by:"]
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/block.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/migration/block.c b/migration/block.c
index 8c6ebafacc..2b9054889a 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -402,7 +402,10 @@ static int init_blk_migration(QEMUFile *f)
         }
 
         sectors = bdrv_nb_sectors(bs);
-        if (sectors <= 0) {
+        if (sectors == 0) {
+            continue;
+        }
+        if (sectors < 0) {
             ret = sectors;
             bdrv_next_cleanup(&it);
             goto out;
-- 
2.44.0

[PULL 07/10] migration: cpr-reboot documentation

[peterx]

From: Steve Sistare <steven.sistare@oracle.com>

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Link: https://lore.kernel.org/r/1710338119-330923-1-git-send-email-steven.sistare@oracle.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 docs/devel/migration/CPR.rst      | 147 ++++++++++++++++++++++++++++++
 docs/devel/migration/features.rst |   1 +
 2 files changed, 148 insertions(+)
 create mode 100644 docs/devel/migration/CPR.rst

diff --git a/docs/devel/migration/CPR.rst b/docs/devel/migration/CPR.rst
new file mode 100644
index 0000000000..63c36470cf
--- /dev/null
+++ b/docs/devel/migration/CPR.rst
@@ -0,0 +1,147 @@
+CheckPoint and Restart (CPR)
+============================
+
+CPR is the umbrella name for a set of migration modes in which the
+VM is migrated to a new QEMU instance on the same host.  It is
+intended for use when the goal is to update host software components
+that run the VM, such as QEMU or even the host kernel.  At this time,
+cpr-reboot is the only available mode.
+
+Because QEMU is restarted on the same host, with access to the same
+local devices, CPR is allowed in certain cases where normal migration
+would be blocked.  However, the user must not modify the contents of
+guest block devices between quitting old QEMU and starting new QEMU.
+
+CPR unconditionally stops VM execution before memory is saved, and
+thus does not depend on any form of dirty page tracking.
+
+cpr-reboot mode
+---------------
+
+In this mode, QEMU stops the VM, and writes VM state to the migration
+URI, which will typically be a file.  After quitting QEMU, the user
+resumes by running QEMU with the ``-incoming`` option.  Because the
+old and new QEMU instances are not active concurrently, the URI cannot
+be a type that streams data from one instance to the other.
+
+Guest RAM can be saved in place if backed by shared memory, or can be
+copied to a file.  The former is more efficient and is therefore
+preferred.
+
+After state and memory are saved, the user may update userland host
+software before restarting QEMU and resuming the VM.  Further, if
+the RAM is backed by persistent shared memory, such as a DAX device,
+then the user may reboot to a new host kernel before restarting QEMU.
+
+This mode supports VFIO devices provided the user first puts the
+guest in the suspended runstate, such as by issuing the
+``guest-suspend-ram`` command to the QEMU guest agent.  The agent
+must be pre-installed in the guest, and the guest must support
+suspend to RAM.  Beware that suspension can take a few seconds, so
+the user should poll to see the suspended state before proceeding
+with the CPR operation.
+
+Usage
+^^^^^
+
+It is recommended that guest RAM be backed with some type of shared
+memory, such as ``memory-backend-file,share=on``, and that the
+``x-ignore-shared`` capability be set.  This combination allows memory
+to be saved in place.  Otherwise, after QEMU stops the VM, all guest
+RAM is copied to the migration URI.
+
+Outgoing:
+  * Set the migration mode parameter to ``cpr-reboot``.
+  * Set the ``x-ignore-shared`` capability if desired.
+  * Issue the ``migrate`` command.  It is recommended the the URI be a
+    ``file`` type, but one can use other types such as ``exec``,
+    provided the command captures all the data from the outgoing side,
+    and provides all the data to the incoming side.
+  * Quit when QEMU reaches the postmigrate state.
+
+Incoming:
+  * Start QEMU with the ``-incoming defer`` option.
+  * Set the migration mode parameter to ``cpr-reboot``.
+  * Set the ``x-ignore-shared`` capability if desired.
+  * Issue the ``migrate-incoming`` command.
+  * If the VM was running when the outgoing ``migrate`` command was
+    issued, then QEMU automatically resumes VM execution.
+
+Example 1
+^^^^^^^^^
+::
+
+  # qemu-kvm -monitor stdio
+  -object memory-backend-file,id=ram0,size=4G,mem-path=/dev/dax0.0,align=2M,share=on -m 4G
+  ...
+
+  (qemu) info status
+  VM status: running
+  (qemu) migrate_set_parameter mode cpr-reboot
+  (qemu) migrate_set_capability x-ignore-shared on
+  (qemu) migrate -d file:vm.state
+  (qemu) info status
+  VM status: paused (postmigrate)
+  (qemu) quit
+
+  ### optionally update kernel and reboot
+  # systemctl kexec
+  kexec_core: Starting new kernel
+  ...
+
+  # qemu-kvm ... -incoming defer
+  (qemu) info status
+  VM status: paused (inmigrate)
+  (qemu) migrate_set_parameter mode cpr-reboot
+  (qemu) migrate_set_capability x-ignore-shared on
+  (qemu) migrate_incoming file:vm.state
+  (qemu) info status
+  VM status: running
+
+Example 2: VFIO
+^^^^^^^^^^^^^^^
+::
+
+  # qemu-kvm -monitor stdio
+  -object memory-backend-file,id=ram0,size=4G,mem-path=/dev/dax0.0,align=2M,share=on -m 4G
+  -device vfio-pci, ...
+  -chardev socket,id=qga0,path=qga.sock,server=on,wait=off
+  -device virtserialport,chardev=qga0,name=org.qemu.guest_agent.0
+  ...
+
+  (qemu) info status
+  VM status: running
+
+  # echo '{"execute":"guest-suspend-ram"}' | ncat --send-only -U qga.sock
+
+  (qemu) info status
+  VM status: paused (suspended)
+  (qemu) migrate_set_parameter mode cpr-reboot
+  (qemu) migrate_set_capability x-ignore-shared on
+  (qemu) migrate -d file:vm.state
+  (qemu) info status
+  VM status: paused (postmigrate)
+  (qemu) quit
+
+  ### optionally update kernel and reboot
+  # systemctl kexec
+  kexec_core: Starting new kernel
+  ...
+
+  # qemu-kvm ... -incoming defer
+  (qemu) info status
+  VM status: paused (inmigrate)
+  (qemu) migrate_set_parameter mode cpr-reboot
+  (qemu) migrate_set_capability x-ignore-shared on
+  (qemu) migrate_incoming file:vm.state
+  (qemu) info status
+  VM status: paused (suspended)
+  (qemu) system_wakeup
+  (qemu) info status
+  VM status: running
+
+Caveats
+^^^^^^^
+
+cpr-reboot mode may not be used with postcopy, background-snapshot,
+or COLO.
diff --git a/docs/devel/migration/features.rst b/docs/devel/migration/features.rst
index 9d1abd2587..d5ca7b86d5 100644
--- a/docs/devel/migration/features.rst
+++ b/docs/devel/migration/features.rst
@@ -11,3 +11,4 @@ Migration has plenty of features to support different use cases.
    vfio
    virtio
    mapped-ram
+   CPR
-- 
2.44.0

[PULL 08/10] migration: Fix iocs leaks during file and fd migration

[peterx]

From: Fabiano Rosas <farosas@suse.de>

The memory for the io channels is being leaked in three different ways
during file migration:

1) if the offset check fails we never drop the ioc reference;

2) we allocate an extra channel for no reason;

3) if multifd is enabled but channel creation fails when calling
   dup(), we leave the previous channels around along with the glib
   polling;

Fix all issues by restructuring the code to first allocate the
channels and only register the watches when all channels have been
created.

For multifd, the file and fd migrations can share code because both
are backed by a QIOChannelFile. For the non-multifd case, the fd needs
to be separate because it is backed by a QIOChannelSocket.

Fixes: 2dd7ee7a51 ("migration/multifd: Add incoming QIOChannelFile support")
Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI")
Reported-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
Link: https://lore.kernel.org/r/20240313212824.16974-2-farosas@suse.de
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/file.h |  1 +
 migration/fd.c   | 29 +++++++-----------------
 migration/file.c | 58 ++++++++++++++++++++++++++++++------------------
 3 files changed, 46 insertions(+), 42 deletions(-)

diff --git a/migration/file.h b/migration/file.h
index 9f71e87f74..7699c04677 100644
--- a/migration/file.h
+++ b/migration/file.h
@@ -20,6 +20,7 @@ void file_start_outgoing_migration(MigrationState *s,
 int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp);
 void file_cleanup_outgoing_migration(void);
 bool file_send_channel_create(gpointer opaque, Error **errp);
+void file_create_incoming_channels(QIOChannel *ioc, Error **errp);
 int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov,
                             int niov, RAMBlock *block, Error **errp);
 int multifd_file_recv_data(MultiFDRecvParams *p, Error **errp);
diff --git a/migration/fd.c b/migration/fd.c
index 4e2a63a73d..39a52e5c90 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -18,6 +18,7 @@
 #include "qapi/error.h"
 #include "channel.h"
 #include "fd.h"
+#include "file.h"
 #include "migration.h"
 #include "monitor/monitor.h"
 #include "io/channel-file.h"
@@ -80,7 +81,6 @@ static gboolean fd_accept_incoming_migration(QIOChannel *ioc,
 void fd_start_incoming_migration(const char *fdname, Error **errp)
 {
     QIOChannel *ioc;
-    QIOChannelFile *fioc;
     int fd = monitor_fd_param(monitor_cur(), fdname, errp);
     if (fd == -1) {
         return;
@@ -94,26 +94,13 @@ void fd_start_incoming_migration(const char *fdname, Error **errp)
         return;
     }
 
-    qio_channel_set_name(ioc, "migration-fd-incoming");
-    qio_channel_add_watch_full(ioc, G_IO_IN,
-                               fd_accept_incoming_migration,
-                               NULL, NULL,
-                               g_main_context_get_thread_default());
-
     if (migrate_multifd()) {
-        int channels = migrate_multifd_channels();
-
-        while (channels--) {
-            fioc = qio_channel_file_new_dupfd(fd, errp);
-            if (!fioc) {
-                return;
-            }
-
-            qio_channel_set_name(ioc, "migration-fd-incoming");
-            qio_channel_add_watch_full(QIO_CHANNEL(fioc), G_IO_IN,
-                                       fd_accept_incoming_migration,
-                                       NULL, NULL,
-                                       g_main_context_get_thread_default());
-        }
+        file_create_incoming_channels(ioc, errp);
+    } else {
+        qio_channel_set_name(ioc, "migration-fd-incoming");
+        qio_channel_add_watch_full(ioc, G_IO_IN,
+                                   fd_accept_incoming_migration,
+                                   NULL, NULL,
+                                   g_main_context_get_thread_default());
     }
 }
diff --git a/migration/file.c b/migration/file.c
index e56c5eb0a5..ddde0ca818 100644
--- a/migration/file.c
+++ b/migration/file.c
@@ -115,13 +115,46 @@ static gboolean file_accept_incoming_migration(QIOChannel *ioc,
     return G_SOURCE_REMOVE;
 }
 
+void file_create_incoming_channels(QIOChannel *ioc, Error **errp)
+{
+    int i, fd, channels = 1;
+    g_autofree QIOChannel **iocs = NULL;
+
+    if (migrate_multifd()) {
+        channels += migrate_multifd_channels();
+    }
+
+    iocs = g_new0(QIOChannel *, channels);
+    fd = QIO_CHANNEL_FILE(ioc)->fd;
+    iocs[0] = ioc;
+
+    for (i = 1; i < channels; i++) {
+        QIOChannelFile *fioc = qio_channel_file_new_dupfd(fd, errp);
+
+        if (!fioc) {
+            while (i) {
+                object_unref(iocs[--i]);
+            }
+            return;
+        }
+
+        iocs[i] = QIO_CHANNEL(fioc);
+    }
+
+    for (i = 0; i < channels; i++) {
+        qio_channel_set_name(iocs[i], "migration-file-incoming");
+        qio_channel_add_watch_full(iocs[i], G_IO_IN,
+                                   file_accept_incoming_migration,
+                                   NULL, NULL,
+                                   g_main_context_get_thread_default());
+    }
+}
+
 void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp)
 {
     g_autofree char *filename = g_strdup(file_args->filename);
     QIOChannelFile *fioc = NULL;
     uint64_t offset = file_args->offset;
-    int channels = 1;
-    int i = 0;
 
     trace_migration_file_incoming(filename);
 
@@ -132,28 +165,11 @@ void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp)
 
     if (offset &&
         qio_channel_io_seek(QIO_CHANNEL(fioc), offset, SEEK_SET, errp) < 0) {
+        object_unref(OBJECT(fioc));
         return;
     }
 
-    if (migrate_multifd()) {
-        channels += migrate_multifd_channels();
-    }
-
-    do {
-        QIOChannel *ioc = QIO_CHANNEL(fioc);
-
-        qio_channel_set_name(ioc, "migration-file-incoming");
-        qio_channel_add_watch_full(ioc, G_IO_IN,
-                                   file_accept_incoming_migration,
-                                   NULL, NULL,
-                                   g_main_context_get_thread_default());
-
-        fioc = qio_channel_file_new_dupfd(fioc->fd, errp);
-
-        if (!fioc) {
-            break;
-        }
-    } while (++i < channels);
+    file_create_incoming_channels(QIO_CHANNEL(fioc), errp);
 }
 
 int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov,
-- 
2.44.0

[PULL 09/10] migration/multifd: Ensure we're not given a socket for file migration

[peterx]

From: Fabiano Rosas <farosas@suse.de>

When doing migration using the fd: URI, QEMU will fetch the file
descriptor passed in via the monitor at
fd_start_outgoing|incoming_migration(), which means the checks at
migration_channels_and_transport_compatible() happen too soon and we
don't know at that point whether the FD refers to a plain file or a
socket.

For this reason, we've been allowing a migration channel of type
SOCKET_ADDRESS_TYPE_FD to pass the initial verifications in scenarios
where the socket migration is not supported, such as with fd + multifd.

The commit decdc76772 ("migration/multifd: Add mapped-ram support to
fd: URI") was supposed to add a second check prior to starting
migration to make sure a socket fd is not passed instead of a file fd,
but failed to do so.

Add the missing verification and update the comment explaining this
situation which is currently incorrect.

Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI")
Signed-off-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Peter Xu <peterx@redhat.com>
Link: https://lore.kernel.org/r/20240315032040.7974-2-farosas@suse.de
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/fd.c        | 8 ++++++++
 migration/file.c      | 7 +++++++
 migration/migration.c | 6 +++---
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/migration/fd.c b/migration/fd.c
index 39a52e5c90..c07030f715 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -22,6 +22,7 @@
 #include "migration.h"
 #include "monitor/monitor.h"
 #include "io/channel-file.h"
+#include "io/channel-socket.h"
 #include "io/channel-util.h"
 #include "options.h"
 #include "trace.h"
@@ -95,6 +96,13 @@ void fd_start_incoming_migration(const char *fdname, Error **errp)
     }
 
     if (migrate_multifd()) {
+        if (fd_is_socket(fd)) {
+            error_setg(errp,
+                       "Multifd migration to a socket FD is not supported");
+            object_unref(ioc);
+            return;
+        }
+
         file_create_incoming_channels(ioc, errp);
     } else {
         qio_channel_set_name(ioc, "migration-fd-incoming");
diff --git a/migration/file.c b/migration/file.c
index ddde0ca818..b6e8ba13f2 100644
--- a/migration/file.c
+++ b/migration/file.c
@@ -15,6 +15,7 @@
 #include "file.h"
 #include "migration.h"
 #include "io/channel-file.h"
+#include "io/channel-socket.h"
 #include "io/channel-util.h"
 #include "options.h"
 #include "trace.h"
@@ -58,6 +59,12 @@ bool file_send_channel_create(gpointer opaque, Error **errp)
     int fd = fd_args_get_fd();
 
     if (fd && fd != -1) {
+        if (fd_is_socket(fd)) {
+            error_setg(errp,
+                       "Multifd migration to a socket FD is not supported");
+            goto out;
+        }
+
         ioc = qio_channel_file_new_dupfd(fd, errp);
     } else {
         ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp);
diff --git a/migration/migration.c b/migration/migration.c
index 644e073b7d..f60bd371e3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -166,9 +166,9 @@ static bool transport_supports_seeking(MigrationAddress *addr)
     }
 
     /*
-     * At this point, the user might not yet have passed the file
-     * descriptor to QEMU, so we cannot know for sure whether it
-     * refers to a plain file or a socket. Let it through anyway.
+     * At this point QEMU has not yet fetched the fd passed in by the
+     * user, so we cannot know for sure whether it refers to a plain
+     * file or a socket. Let it through anyway and check at fd.c.
      */
     if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
         return addr->u.socket.type == SOCKET_ADDRESS_TYPE_FD;
-- 
2.44.0

[PULL 10/10] migration/multifd: Duplicate the fd for the outgoing_args

[peterx]

From: Fabiano Rosas <farosas@suse.de>

We currently store the file descriptor used during the main outgoing
channel creation to use it again when creating the multifd
channels.

Since this fd is used for the first iochannel, there's risk that the
QIOChannel gets freed and the fd closed while outgoing_args.fd still
has it available. This could lead to an fd-reuse bug.

Duplicate the outgoing_args fd to avoid this issue.

Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Peter Xu <peterx@redhat.com>
Link: https://lore.kernel.org/r/20240315032040.7974-3-farosas@suse.de
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/fd.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/migration/fd.c b/migration/fd.c
index c07030f715..fe0d096abd 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -49,8 +49,7 @@ void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **
 {
     QIOChannel *ioc;
     int fd = monitor_get_fd(monitor_cur(), fdname, errp);
-
-    outgoing_args.fd = -1;
+    int newfd;
 
     if (fd == -1) {
         return;
@@ -63,7 +62,17 @@ void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **
         return;
     }
 
-    outgoing_args.fd = fd;
+    /*
+     * This is dup()ed just to avoid referencing an fd that might
+     * be already closed by the iochannel.
+     */
+    newfd = dup(fd);
+    if (newfd == -1) {
+        error_setg_errno(errp, errno, "Could not dup FD %d", fd);
+        object_unref(ioc);
+        return;
+    }
+    outgoing_args.fd = newfd;
 
     qio_channel_set_name(ioc, "migration-fd-outgoing");
     migration_channel_connect(s, ioc, NULL, NULL);
-- 
2.44.0

Re: [PULL 00/10] Migration 20240317 patches

[Peter Maydell]

On Sun, 17 Mar 2024 at 20:58, <peterx@redhat.com> wrote:
>
> From: Peter Xu <peterx@redhat.com>
>
> The following changes since commit 35ac6831d98e18e2c78c85c93e3a6ca1f1ae3e58:
>
>   Merge tag 'net-pull-request' of https://github.com/jasowang/qemu into staging (2024-03-12 13:42:57 +0000)
>
> are available in the Git repository at:
>
>   https://gitlab.com/peterx/qemu.git tags/migration-20240317-pull-request
>
> for you to fetch changes up to 9adfb308c1513562d6acec02aa780c5ef9b0193d:
>
>   migration/multifd: Duplicate the fd for the outgoing_args (2024-03-15 11:26:33 -0400)
>
> ----------------------------------------------------------------
> Migration pull for 9.0-rc0
>
> - Nicholas/Phil's fix on migration corruption / inconsistent for tcg
> - Cedric's fix on block migration over n_sectors==0
> - Steve's CPR reboot documentation page
> - Fabiano's misc fixes on mapped-ram (IOC leak, dup() errors, fd checks, fd
>   use race, etc.)


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/9.0
for any user-visible changes.

-- PMM