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

["Cédric Le Goater" <clg@redhat.com>]

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() ?


> 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.

Thanks,

C.