From c827fafcaad3e8b3dcf7eeb5944b03f6b63dfc44 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Mon, 11 Mar 2024 20:33:35 -0300 Subject: migration: Fix error handling after dup in file migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Fabiano Rosas Reviewed-by: "Daniel P. Berrangé" Link: https://lore.kernel.org/r/20240311233335.17299-3-farosas@suse.de Signed-off-by: Peter Xu --- migration/fd.c | 9 ++++----- migration/file.c | 14 +++++++------- 2 files changed, 11 insertions(+), 12 deletions(-) (limited to 'migration') 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); -- cgit v1.2.3 From 2e128776dc56f502c2ee41750afe83938f389528 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= Date: Tue, 12 Mar 2024 13:04:31 +0100 Subject: migration: Skip only empty block devices MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Cc: qemu-stable Suggested-by: Kevin Wolf Fixes: fea68bb6e9fa ("block: Eliminate bdrv_iterate(), use bdrv_next()") Signed-off-by: Cédric Le Goater Reviewed-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf Link: https://lore.kernel.org/r/20240312120431.550054-1-clg@redhat.com [peterx: fix "Suggested-by:"] Signed-off-by: Peter Xu --- migration/block.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'migration') 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; -- cgit v1.2.3 From 74228c598f139bd9ce7839794be9a3ccc180fb27 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Wed, 13 Mar 2024 18:28:23 -0300 Subject: migration: Fix iocs leaks during file and fd migration 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 Signed-off-by: Fabiano Rosas Link: https://lore.kernel.org/r/20240313212824.16974-2-farosas@suse.de Signed-off-by: Peter Xu --- migration/fd.c | 29 ++++++++-------------------- migration/file.c | 58 ++++++++++++++++++++++++++++++++++++-------------------- migration/file.h | 1 + 3 files changed, 46 insertions(+), 42 deletions(-) (limited to 'migration') 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, 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); -- cgit v1.2.3 From 73f6f9a12fb4a3afe01e18690ebd6a6e4283c1a6 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Fri, 15 Mar 2024 00:20:38 -0300 Subject: migration/multifd: Ensure we're not given a socket for file migration 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 Reviewed-by: Peter Xu Link: https://lore.kernel.org/r/20240315032040.7974-2-farosas@suse.de Signed-off-by: Peter Xu --- migration/fd.c | 8 ++++++++ migration/file.c | 7 +++++++ migration/migration.c | 6 +++--- 3 files changed, 18 insertions(+), 3 deletions(-) (limited to 'migration') 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; -- cgit v1.2.3 From 9adfb308c1513562d6acec02aa780c5ef9b0193d Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Fri, 15 Mar 2024 00:20:39 -0300 Subject: migration/multifd: Duplicate the fd for the outgoing_args 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 Signed-off-by: Fabiano Rosas Reviewed-by: Peter Xu Link: https://lore.kernel.org/r/20240315032040.7974-3-farosas@suse.de Signed-off-by: Peter Xu --- migration/fd.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) (limited to 'migration') 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); -- cgit v1.2.3