| Message ID | 20240313135018.28079-1-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Fix iocs leaks during file and fd migration | expand |
On Wed, Mar 13, 2024 at 10:50:18AM -0300, Fabiano Rosas wrote: > 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) if multifd is not enabled, we allocate an extra channel for no > reason; This leak should also happen even if multifd enabled; it just always leak one unconditionally.. > > 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. > > The file and fd migrations can share this code because both are backed > by the file migration infrastructure. > > 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> > --- > based-on: https://gitlab.com/peterx/migration-stable Invalid link. :) I am not sure whether patchew will understand this even if the link is correct, but I hope! > CI run: https://gitlab.com/farosas/qemu/-/pipelines/1211909831 > --- > migration/fd.c | 28 +++------------------- > migration/file.c | 62 +++++++++++++++++++++++++++++++----------------- > migration/file.h | 3 +++ > 3 files changed, 46 insertions(+), 47 deletions(-) > > diff --git a/migration/fd.c b/migration/fd.c > index 4e2a63a73d..b74a3eb8c8 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" > @@ -68,19 +69,9 @@ void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error ** > object_unref(OBJECT(ioc)); > } > > -static gboolean fd_accept_incoming_migration(QIOChannel *ioc, > - GIOCondition condition, > - gpointer opaque) > -{ > - migration_channel_process_incoming(ioc); > - object_unref(OBJECT(ioc)); > - return G_SOURCE_REMOVE; > -} > - > 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; > @@ -96,24 +87,11 @@ void fd_start_incoming_migration(const char *fdname, Error **errp) > > qio_channel_set_name(ioc, "migration-fd-incoming"); > qio_channel_add_watch_full(ioc, G_IO_IN, > - fd_accept_incoming_migration, > + file_accept_incoming_migration, > NULL, NULL, > g_main_context_get_thread_default()); Mostly correct, but IIUC we'll still leak this main io watch if below failed, similar issue for file path below. We may need to manage all IOCs including the main one. > > 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_recv_channels_create(fd, errp); > } > } > diff --git a/migration/file.c b/migration/file.c > index e56c5eb0a5..bb264115eb 100644 > --- a/migration/file.c > +++ b/migration/file.c > @@ -106,22 +106,48 @@ void file_start_outgoing_migration(MigrationState *s, > migration_channel_connect(s, ioc, NULL, NULL); > } > > -static gboolean file_accept_incoming_migration(QIOChannel *ioc, > - GIOCondition condition, > - gpointer opaque) > +gboolean file_accept_incoming_migration(QIOChannel *ioc, GIOCondition condition, > + gpointer opaque) > { > migration_channel_process_incoming(ioc); > object_unref(OBJECT(ioc)); > return G_SOURCE_REMOVE; > } > > +void file_recv_channels_create(int fd, Error **errp) > +{ > + int i, channels = migrate_multifd_channels(); > + g_autofree QIOChannelFile **fiocs = g_new0(QIOChannelFile *, channels); > + > + for (i = 0; i < channels; i++) { > + QIOChannelFile *fioc = qio_channel_file_new_dupfd(fd, errp); > + > + if (!fioc) { > + while (i) { > + object_unref(fiocs[--i]); > + } > + return; > + } > + > + fiocs[i] = fioc; > + } > + > + for (i = 0; i < channels; i++) { > + QIOChannelFile *fioc = fiocs[i]; > + > + qio_channel_set_name(QIO_CHANNEL(fioc), "migration-file-incoming"); > + qio_channel_add_watch_full(QIO_CHANNEL(fioc), 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 +158,20 @@ 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; > } > > + qio_channel_set_name(QIO_CHANNEL(fioc), "migration-file-incoming"); > + qio_channel_add_watch_full(QIO_CHANNEL(fioc), G_IO_IN, > + file_accept_incoming_migration, > + NULL, NULL, > + g_main_context_get_thread_default()); > + > + > if (migrate_multifd()) { > - channels += migrate_multifd_channels(); > + file_recv_channels_create(fioc->fd, errp); > } > - > - 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); > } > > int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov, > diff --git a/migration/file.h b/migration/file.h > index 9f71e87f74..940122db0f 100644 > --- a/migration/file.h > +++ b/migration/file.h > @@ -20,6 +20,9 @@ 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_recv_channels_create(int fd, Error **errp); > +gboolean file_accept_incoming_migration(QIOChannel *ioc, GIOCondition condition, > + gpointer opaque); > 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); > -- > 2.35.3 >
Peter Xu <peterx@redhat.com> writes: > On Wed, Mar 13, 2024 at 10:50:18AM -0300, Fabiano Rosas wrote: >> 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) if multifd is not enabled, we allocate an extra channel for no >> reason; > > This leak should also happen even if multifd enabled; it just always leak > one unconditionally.. > >> >> 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. >> >> The file and fd migrations can share this code because both are backed >> by the file migration infrastructure. >> >> 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> >> --- >> based-on: https://gitlab.com/peterx/migration-stable > > Invalid link. :) lol, all kinds of wrong. The correct one would be: https://gitlab.com/peterx/qemu/-/commits/migration-stable > > I am not sure whether patchew will understand this even if the link is > correct, but I hope! I was thinking more of helping reviewers than patchew actually. > >> CI run: https://gitlab.com/farosas/qemu/-/pipelines/1211909831 >> --- >> migration/fd.c | 28 +++------------------- >> migration/file.c | 62 +++++++++++++++++++++++++++++++----------------- >> migration/file.h | 3 +++ >> 3 files changed, 46 insertions(+), 47 deletions(-) >> >> diff --git a/migration/fd.c b/migration/fd.c >> index 4e2a63a73d..b74a3eb8c8 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" >> @@ -68,19 +69,9 @@ void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error ** >> object_unref(OBJECT(ioc)); >> } >> >> -static gboolean fd_accept_incoming_migration(QIOChannel *ioc, >> - GIOCondition condition, >> - gpointer opaque) >> -{ >> - migration_channel_process_incoming(ioc); >> - object_unref(OBJECT(ioc)); >> - return G_SOURCE_REMOVE; >> -} >> - >> 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; >> @@ -96,24 +87,11 @@ void fd_start_incoming_migration(const char *fdname, Error **errp) >> >> qio_channel_set_name(ioc, "migration-fd-incoming"); >> qio_channel_add_watch_full(ioc, G_IO_IN, >> - fd_accept_incoming_migration, >> + file_accept_incoming_migration, >> NULL, NULL, >> g_main_context_get_thread_default()); > > Mostly correct, but IIUC we'll still leak this main io watch if below > failed, similar issue for file path below. We may need to manage all IOCs > including the main one. > Yes, good point. Sigh... I'll get it right eventually. >> >> 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_recv_channels_create(fd, errp); >> } >> } >> diff --git a/migration/file.c b/migration/file.c >> index e56c5eb0a5..bb264115eb 100644 >> --- a/migration/file.c >> +++ b/migration/file.c >> @@ -106,22 +106,48 @@ void file_start_outgoing_migration(MigrationState *s, >> migration_channel_connect(s, ioc, NULL, NULL); >> } >> >> -static gboolean file_accept_incoming_migration(QIOChannel *ioc, >> - GIOCondition condition, >> - gpointer opaque) >> +gboolean file_accept_incoming_migration(QIOChannel *ioc, GIOCondition condition, >> + gpointer opaque) >> { >> migration_channel_process_incoming(ioc); >> object_unref(OBJECT(ioc)); >> return G_SOURCE_REMOVE; >> } >> >> +void file_recv_channels_create(int fd, Error **errp) >> +{ >> + int i, channels = migrate_multifd_channels(); >> + g_autofree QIOChannelFile **fiocs = g_new0(QIOChannelFile *, channels); >> + >> + for (i = 0; i < channels; i++) { >> + QIOChannelFile *fioc = qio_channel_file_new_dupfd(fd, errp); >> + >> + if (!fioc) { >> + while (i) { >> + object_unref(fiocs[--i]); >> + } >> + return; >> + } >> + >> + fiocs[i] = fioc; >> + } >> + >> + for (i = 0; i < channels; i++) { >> + QIOChannelFile *fioc = fiocs[i]; >> + >> + qio_channel_set_name(QIO_CHANNEL(fioc), "migration-file-incoming"); >> + qio_channel_add_watch_full(QIO_CHANNEL(fioc), 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 +158,20 @@ 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; >> } >> >> + qio_channel_set_name(QIO_CHANNEL(fioc), "migration-file-incoming"); >> + qio_channel_add_watch_full(QIO_CHANNEL(fioc), G_IO_IN, >> + file_accept_incoming_migration, >> + NULL, NULL, >> + g_main_context_get_thread_default()); >> + >> + >> if (migrate_multifd()) { >> - channels += migrate_multifd_channels(); >> + file_recv_channels_create(fioc->fd, errp); >> } >> - >> - 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); >> } >> >> int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov, >> diff --git a/migration/file.h b/migration/file.h >> index 9f71e87f74..940122db0f 100644 >> --- a/migration/file.h >> +++ b/migration/file.h >> @@ -20,6 +20,9 @@ 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_recv_channels_create(int fd, Error **errp); >> +gboolean file_accept_incoming_migration(QIOChannel *ioc, GIOCondition condition, >> + gpointer opaque); >> 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); >> -- >> 2.35.3 >>
diff --git a/migration/fd.c b/migration/fd.c index 4e2a63a73d..b74a3eb8c8 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" @@ -68,19 +69,9 @@ void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error ** object_unref(OBJECT(ioc)); } -static gboolean fd_accept_incoming_migration(QIOChannel *ioc, - GIOCondition condition, - gpointer opaque) -{ - migration_channel_process_incoming(ioc); - object_unref(OBJECT(ioc)); - return G_SOURCE_REMOVE; -} - 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; @@ -96,24 +87,11 @@ void fd_start_incoming_migration(const char *fdname, Error **errp) qio_channel_set_name(ioc, "migration-fd-incoming"); qio_channel_add_watch_full(ioc, G_IO_IN, - fd_accept_incoming_migration, + file_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_recv_channels_create(fd, errp); } } diff --git a/migration/file.c b/migration/file.c index e56c5eb0a5..bb264115eb 100644 --- a/migration/file.c +++ b/migration/file.c @@ -106,22 +106,48 @@ void file_start_outgoing_migration(MigrationState *s, migration_channel_connect(s, ioc, NULL, NULL); } -static gboolean file_accept_incoming_migration(QIOChannel *ioc, - GIOCondition condition, - gpointer opaque) +gboolean file_accept_incoming_migration(QIOChannel *ioc, GIOCondition condition, + gpointer opaque) { migration_channel_process_incoming(ioc); object_unref(OBJECT(ioc)); return G_SOURCE_REMOVE; } +void file_recv_channels_create(int fd, Error **errp) +{ + int i, channels = migrate_multifd_channels(); + g_autofree QIOChannelFile **fiocs = g_new0(QIOChannelFile *, channels); + + for (i = 0; i < channels; i++) { + QIOChannelFile *fioc = qio_channel_file_new_dupfd(fd, errp); + + if (!fioc) { + while (i) { + object_unref(fiocs[--i]); + } + return; + } + + fiocs[i] = fioc; + } + + for (i = 0; i < channels; i++) { + QIOChannelFile *fioc = fiocs[i]; + + qio_channel_set_name(QIO_CHANNEL(fioc), "migration-file-incoming"); + qio_channel_add_watch_full(QIO_CHANNEL(fioc), 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 +158,20 @@ 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; } + qio_channel_set_name(QIO_CHANNEL(fioc), "migration-file-incoming"); + qio_channel_add_watch_full(QIO_CHANNEL(fioc), G_IO_IN, + file_accept_incoming_migration, + NULL, NULL, + g_main_context_get_thread_default()); + + if (migrate_multifd()) { - channels += migrate_multifd_channels(); + file_recv_channels_create(fioc->fd, errp); } - - 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); } int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov, diff --git a/migration/file.h b/migration/file.h index 9f71e87f74..940122db0f 100644 --- a/migration/file.h +++ b/migration/file.h @@ -20,6 +20,9 @@ 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_recv_channels_create(int fd, Error **errp); +gboolean file_accept_incoming_migration(QIOChannel *ioc, GIOCondition condition, + gpointer opaque); 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);
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) if multifd is not enabled, 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. The file and fd migrations can share this code because both are backed by the file migration infrastructure. 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> --- based-on: https://gitlab.com/peterx/migration-stable CI run: https://gitlab.com/farosas/qemu/-/pipelines/1211909831 --- migration/fd.c | 28 +++------------------- migration/file.c | 62 +++++++++++++++++++++++++++++++----------------- migration/file.h | 3 +++ 3 files changed, 46 insertions(+), 47 deletions(-)