Re: [PATCH v2 0/2] migration mapped-ram fixes

[Peter Xu <peterx@redhat.com>]

On Thu, Mar 14, 2024 at 01:55:31PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Mar 13, 2024 at 06:28:22PM -0300, Fabiano Rosas wrote:
> >> Hi,
> >> 
> >> In this v2:
> >> 
> >> patch 1 - The fix for the ioc leaks, now including the main channel
> >> 
> >> patch 2 - A fix for an fd: migration case I thought I had written code
> >>           for, but obviously didn't.
> >
> > Maybe I found one more issue.. I'm looking at fd_start_outgoing_migration().
> >
> >     ioc = qio_channel_new_fd(fd, errp);  <----- here the fd is consumed and
> >                                                 then owned by the IOC
> >     if (!ioc) {
> >         close(fd);
> >         return;
> >     }
> >
> >     outgoing_args.fd = fd;               <----- here we use the fd again,
> >                                                 and "owned" by outgoing_args
> >                                                 even if it shouldn't?
> >
> > The problem is outgoing_args.fd will be cleaned up with a close().  I had a
> > feeling that it's possible it will close() something else if the fd reused
> > before that close() but after the IOC's.  We may want yet another dup() for
> > outgoing_args.fd?
> 
> I think the right fix is to not close() it at
> fd_cleanup_outgoing_migration(). That fd is already owned by the ioc.

But outgoing_args.fd can point to other things if the IOC (along with the
ioc->fd) is released.  Keeping outgoing_args.fd pointing to that fd index
should be dangerous because the integer can be reused.

> 
> >
> > If you agree, we may also want to avoid doing:
> >
> >     outgoing_args.fd = -1;
> 
> We will always need this. This is just initialization of the field
> because 0 is a valid fd value. Otherwise the file.c code can't know if
> we're actually using an fd at all.

I meant avoid setting it to -1 only in fd_start_outgoing_migration().
Using -1 to represent "no fd" is fine.

> 
> @file_send_channel_create:
> 
> int fd = fd_args_get_fd();
> 
> if (fd && fd != -1) {
>     <new IOC from fd>
> } else {
>     <new IOC from file name>
> }
> 
> >
> > We could assert it instead making sure no fd leak.
> >
> >> 
> >> Thank you for your patience.
> >> 
> >> based-on: https://gitlab.com/peterx/qemu/-/commits/migration-stable
> >> CI run: https://gitlab.com/farosas/qemu/-/pipelines/1212483701
> >> 
> >> Fabiano Rosas (2):
> >>   migration: Fix iocs leaks during file and fd migration
> >>   migration/multifd: Ensure we're not given a socket for file migration
> >> 
> >>  migration/fd.c   | 35 +++++++++++---------------
> >>  migration/file.c | 65 ++++++++++++++++++++++++++++++++----------------
> >>  migration/file.h |  1 +
> >>  3 files changed, 60 insertions(+), 41 deletions(-)
> >> 
> >> -- 
> >> 2.35.3
> >> 
> 

-- 
Peter Xu