Re: [PATCH 2/3] vfio/migration: Refactor vfio_vmstate_change/_prepare() error reporting

[Peter Xu <peterx@redhat.com>]

On Tue, Oct 22, 2024 at 03:24:56PM +0200, Cédric Le Goater wrote:
> On 10/22/24 11:38, Avihai Horon wrote:
> > 
> > On 21/10/2024 19:54, Peter Xu wrote:
> > > External email: Use caution opening links or attachments
> > > 
> > > 
> > > On Mon, Oct 21, 2024 at 06:43:13PM +0200, Cédric Le Goater wrote:
> > > > Hello,
> > > > 
> > > > > IIUC the migration thread should always see valid migration object, as it
> > > > > takes one refcount at the entrance of migration_thread():
> > > > > 
> > > > >       object_ref(OBJECT(s));
> > > > Could the migration have failed before ? in migrate_fd_connect()
> > > I just noticed it's a vm state change notifier..
> > 
> > Yep.
> > I stumbled upon this bug by accident when running on a buggy machine.
> > Migration wasn't involved, I just started the VM, shut it down and got the assert (as my VFIO device was faulty and errored on RUNNING->STOP state change).
> > 
> > You can repro it by forcefully triggering an error on *->STOP transition:
> > 
> > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> > index 17199b73ae..d41cb7c634 100644
> > --- a/hw/vfio/migration.c
> > +++ b/hw/vfio/migration.c
> > @@ -831,7 +831,9 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
> >       }
> > 
> >       ret = vfio_migration_set_state_or_reset(vbasedev, new_state, &local_err);
> > -    if (ret) {
> > +    if (ret || new_state == VFIO_DEVICE_STATE_STOP) {
> > +        ret = -1;
> > +        error_setg(&local_err, "%s: forced error", vbasedev->name);
> >           /*
> >            * Migration should be aborted in this case, but vm_state_notify()
> >            * currently does not support reporting failures.
> > 
> > > 
> > > If so, maybe VFIO could refer to its internal states showing that it's
> > > during a migration first (so as to guarantee the migration object is valid;
> > > e.g., only after save_setup() but before save_cleanup() hooks are invoked).
> > 
> > It's an option, but I think it's a bit awkward as we'd need to check
> > that VFIOMigration->data_buffer is set
> 
> That's what I was looking at too. It works. It feels a bit awkward
> indeed. We could hide the details in an helper routine though.
> 
> > or add a new flag.
> 
> yes that's another solution.
> 
> Peter,
> 
> I wonder if we could grab a ref on current_migration in save_setup(),
> store it under VFIOMigration and drop it save_cleanup() ?

VFIO can definitely do that, but I am not sure how that would help.. as I
think the migration object can never go away anyway during setup->cleanup,
so taking that extra refcount shouldn't change anything.

IOW, AFAICT this bug is triggered only when without migration at all.

> 
> 
> > Besides that, as Cedric pointed out, VFIO code calls migration_is_setup_or_active() which can also be unsafe, as it might be invoked after migration object has been freed.
> > 
> > Maybe a simpler solution would be to extend the the migration object lifetime?
> > Looking at commit history, you can see that commit 1f8956041ad3 ("migration: finalize current_migration object") added migration object finalization at the very end of qemu cleanup.
> > Then came commit 892ae715b6bc ("migration: Cleanup during exit") and moved the migration object finalization to the beginning of qemu cleanup (before stopping the VM etc.).
> > 
> > If so, the fix could be something like the below?
> > 
> > -------------8<-------------
> > diff --git a/include/migration/misc.h b/include/migration/misc.h
> > index bfadc5613b..5eb099349a 100644
> > --- a/include/migration/misc.h
> > +++ b/include/migration/misc.h
> > @@ -52,6 +52,7 @@ void dump_vmstate_json_to_file(FILE *out_fp);
> > 
> >   /* migration/migration.c */
> >   void migration_object_init(void);
> > +void migration_object_finalize(void);
> >   void migration_shutdown(void);
> >   bool migration_is_idle(void);
> >   bool migration_is_active(void);
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 021faee2f3..9eaa7947bc 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -265,6 +265,11 @@ void migration_object_init(void)
> >       dirty_bitmap_mig_init();
> >   }
> > 
> > +void migration_object_finalize(void)
> > +{
> > +    object_unref(OBJECT(current_migration));
> > +}
> > +
> >   typedef struct {
> >       QEMUBH *bh;
> >       QEMUBHFunc *cb;
> > @@ -330,7 +335,6 @@ void migration_shutdown(void)
> >        * stop the migration using this structure
> >        */
> >       migration_cancel(NULL);
> > -    object_unref(OBJECT(current_migration));
> > 
> >       /*
> >        * Cancel outgoing migration of dirty bitmaps. It should
> > diff --git a/system/runstate.c b/system/runstate.c
> > index c2c9afa905..fa823f5e72 100644
> > --- a/system/runstate.c
> > +++ b/system/runstate.c
> > @@ -930,5 +930,6 @@ void qemu_cleanup(int status)
> >       monitor_cleanup();
> >       qemu_chr_cleanup();
> >       user_creatable_cleanup();
> > +    migration_object_finalize();
> >       /* TODO: unref root container, check all devices are ok */
> >   }
> > -------------8<-------------
> 
> 892ae715b6bc was trying to fix potential use-after-free issues.
> 
> I think it is safer to introduce an helper routine checking
> (in some ways) if a migration is in progress than partially
> revert 892ae715b6bc.

Right, Dave also mentioned something in 892ae715b6bc about moving it
earlier:

    We do this a bit earlier; so hopefully migration gets out of the way
    before all the devices etc are freed.

But I don't know the relationship on device free() v.s. the migration
object.  We might at least want to figure that out if we want to move it
again.

I notice that vdpa also started using migration_is_setup_or_active(), so if
it's used in more places, maybe indeed we can consider making them safe to
be called at any phase of QEMU.

Logically it is safe in vm state change hook always because it has the BQL
and the object can only be freed when with BQL.

So let me send a small patch later to hopefully make all these exported
functions (including migration_file_set_error() in this case, logically
anything in migration/misc.h) safe to be called without migration.

-- 
Peter Xu