Re: [PATCH] migration: Fix a possible crash when halting a guest during migration

[Thomas Huth <thuth@redhat.com>]

On 15/12/2025 17.12, Peter Xu wrote:
> On Mon, Dec 15, 2025 at 04:28:39PM +0100, Kevin Wolf wrote:
>> Am 15.12.2025 um 16:11 hat Thomas Huth geschrieben:
>>> On 15/12/2025 14.42, Kevin Wolf wrote:
>>>> Am 12.12.2025 um 22:26 hat Fabiano Rosas geschrieben:
>>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>>
>>>>>> On 08/12/2025 16.26, Fabiano Rosas wrote:
>>>>>>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>>>>>>>
>>>>>>>> On Mon, Dec 08, 2025 at 02:51:01PM +0100, Thomas Huth wrote:
>>>>>>>>> From: Thomas Huth <thuth@redhat.com>
>>>>>>>>>
>>>>>>>>> When shutting down a guest that is currently in progress of being
>>>>>>>>> migrated, there is a chance that QEMU might crash during bdrv_delete().
>>>>>>>>> The backtrace looks like this:
>>>>>>>>>
>>>>>>>>>     Thread 74 "mig/src/main" received signal SIGSEGV, Segmentation fault.
>>>>>>>>>
>>>>>>>>>     [Switching to Thread 0x3f7de7fc8c0 (LWP 2161436)]
>>>>>>>>>     0x000002aa00664012 in bdrv_delete (bs=0x2aa00f875c0) at ../../devel/qemu/block.c:5560
>>>>>>>>>     5560	        QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list);
>>>>>>>>>     (gdb) bt
>>>>>>>>>     #0  0x000002aa00664012 in bdrv_delete (bs=0x2aa00f875c0) at ../../devel/qemu/block.c:5560
>>>>>>>>>     #1  bdrv_unref (bs=0x2aa00f875c0) at ../../devel/qemu/block.c:7170
>>>>>>>>>     Backtrace stopped: Cannot access memory at address 0x3f7de7f83e0
>>>>>>>>>
>>>>>>>
>>>>>>> How does the migration thread reaches here? Is this from
>>>>>>> migration_block_inactivate()?
>>>>>>
>>>>>> Unfortunately, gdb was not very helpful here (claiming that it cannot access
>>>>>> the memory and stack anymore), so I had to do some printf debugging. This is
>>>>>> what seems to happen:
>>>>>>
>>>>>> Main thread: qemu_cleanup() calls  migration_shutdown() -->
>>>>>> migration_cancel() which signals the migration thread to cancel the migration.
>>>>>>
>>>>>> Migration thread: migration_thread() got kicked out the loop and calls
>>>>>> migration_iteration_finish(), which tries to get the BQL via bql_lock() but
>>>>>> that is currently held by another thread, so the migration thread is blocked
>>>>>> here.
>>>>>>
>>>>>> Main thread: qemu_cleanup() advances to bdrv_close_all() that uses
>>>>>> blockdev_close_all_bdrv_states() to unref all BDS. The BDS with the name
>>>>>> 'libvirt-1-storage' gets deleted via bdrv_delete() that way.
>>>>>>
>>>>>
>>>>> Has qmp_blockdev_del() ever been called to remove the BDS from the
>>>>> monitor_bdrv_states list? Otherwise your debugging seems to indicate
>>>>> blockdev_close_all_bdrv_states() is dropping the last reference to bs,
>>>>> but it's still accessible from bdrv_next() via
>>>>> bdrv_next_monitor_owned().
>>>>
>>>> The reference that blockdev_close_all_bdrv_states() drops is the monitor
>>>> reference. So is this the right fix (completely untested, but matches
>>>> what qmp_blockdev_del() does)?
>>>>
>>>> Kevin
>>>>
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index dbd1d4d3e80..6e86c6262f9 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -686,6 +686,7 @@ void blockdev_close_all_bdrv_states(void)
>>>>
>>>>        GLOBAL_STATE_CODE();
>>>>        QTAILQ_FOREACH_SAFE(bs, &monitor_bdrv_states, monitor_list, next_bs) {
>>>> +        QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
>>>>            bdrv_unref(bs);
>>>>        }
>>>>    }
>>>
>>> Thanks a lot, Kevin! This looks like the right fix for me - I gave it
>>> a try and it fixes the crash indeed!
>>
>> Good. I think something like your patch would still be good for 11.0.
>> Having undefined order in shutdown is just asking for trouble. So it
>> would be good if we could be sure that migration is out of the way when
>> migration_shutdown() returns.
>>
>> I sent the above as a proper patch to fix the immediate problem for
>> 10.2.
> 
> Thanks all!
> 
> Does that completely fix the problem?

Yes, Kevin's patch fixes the crash!

>  If so, IMHO we don't need the
> migration change anymore until later.  As replied in the other email, it
> was at least intentional a few years ago (when introducing
> migration_shutdown()) to not join() the migration thread here.

Fine for me.

  Thomas