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

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

On 10/22/24 16:29, Peter Xu wrote:
> 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.

It won't be but we would have a VFIOMigration::MigrationState pointer to
test in the vmstate change handler, which is a bit cleaner than testing
migration->data_buffer. Just an idea. I am not convinced myself either.

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

OK.

Thanks

C.