[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] virtio-net: failover: add missing remove_migration_state_cha
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH] virtio-net: failover: add missing remove_migration_state_change_notifier() |
Date: |
Tue, 27 Apr 2021 16:16:44 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 |
On 4/27/21 4:02 PM, Dr. David Alan Gilbert wrote:
> * Laurent Vivier (lvivier@redhat.com) wrote:
>> In the failover case configuration, virtio_net_device_realize() uses an
>> add_migration_state_change_notifier() to add a state notifier, but this
>> notifier is not removed by the unrealize function when the virtio-net
>> card is unplugged.
>>
>> If the card is unplugged and a migration is started, the notifier is
>> called and as it is not valid anymore QEMU crashes.
>>
>> This patch fixes the problem by adding the
>> remove_migration_state_change_notifier() in virtio_net_device_unrealize().
>>
>> The problem can be reproduced with:
>>
>> $ qemu-system-x86_64 -enable-kvm -m 1g -M q35 \
>> -device pcie-root-port,slot=4,id=root1 \
>> -device pcie-root-port,slot=5,id=root2 \
>> -device
>> virtio-net-pci,id=net1,mac=52:54:00:6f:55:cc,failover=on,bus=root1 \
>> -monitor stdio disk.qcow2
>> (qemu) device_del net1
>> (qemu) migrate "exec:gzip -c > STATEFILE.gz"
>>
>> Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
>> 0x0000000000000000 in ?? ()
>> (gdb) bt
>> #0 0x0000000000000000 in ()
>> #1 0x0000555555d726d7 in notifier_list_notify (...)
>> at .../util/notify.c:39
>> #2 0x0000555555842c1a in migrate_fd_connect (...)
>> at .../migration/migration.c:3975
>> #3 0x0000555555950f7d in migration_channel_connect (...)
>> error@entry=0x0) at .../migration/channel.c:107
>> #4 0x0000555555910922 in exec_start_outgoing_migration (...)
>> at .../migration/exec.c:42
>>
>> Reported-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>
> Yep, I think that's OK.
IIUC HMP device_del ends calling device_finalize(), and per Igor's
explanation of qdev transition states:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg661853.html
I'd expect this to be done in instance_init/instance_finalize,
but since 'failover' is a virtio_net_properties, the callbacks
has to be registered in the realize() function, thus unregistered
in unrealize().
Mentioning it is in realize/unrealize due to 'failover' being a property:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
>> ---
>> hw/net/virtio-net.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 66b9ff451185..914051feb75b 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -3373,6 +3373,7 @@ static void virtio_net_device_unrealize(DeviceState
>> *dev)
>>
>> if (n->failover) {
>> device_listener_unregister(&n->primary_listener);
>> + remove_migration_state_change_notifier(&n->migration_state);
>> }
>>
>> max_queues = n->multiqueue ? n->max_queues : 1;
>> --
>> 2.30.2
>>