[Qemu-devel] [PATCH] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable

[Qemu-devel] [PATCH] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable

[Ashijeet Acharya]

Commit a3a3d8c7 introduced a segfault bug while checking for
'dc->vmsd->unmigratable' which caused QEMU to crash when trying to add
devices which do no set their 'dc->vmsd' yet while initialization.
Place a 'dc->vmsd' check prior to it so that we do not segfault for
such devices.

NOTE: This doesn't compromise the functioning of --only-migratable
option as all the unmigratable devices do set their 'dc->vmsd'.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 qdev-monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 81d01df..a1106fd 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -578,7 +578,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
         return NULL;
     }
 
-    if (only_migratable) {
+    if (only_migratable && dc->vmsd) {
         if (dc->vmsd->unmigratable) {
             error_setg(errp, "Device %s is not migratable, but "
                        "--only-migratable was specified", driver);
-- 
2.6.2

Re: [Qemu-devel] [PATCH] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable

[Peter Maydell]

On 30 January 2017 at 14:41, Ashijeet Acharya <ashijeetacharya@gmail.com> wrote:
> Commit a3a3d8c7 introduced a segfault bug while checking for
> 'dc->vmsd->unmigratable' which caused QEMU to crash when trying to add
> devices which do no set their 'dc->vmsd' yet while initialization.
> Place a 'dc->vmsd' check prior to it so that we do not segfault for
> such devices.
>
> NOTE: This doesn't compromise the functioning of --only-migratable
> option as all the unmigratable devices do set their 'dc->vmsd'.
>
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
>  qdev-monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 81d01df..a1106fd 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -578,7 +578,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>          return NULL;
>      }
>
> -    if (only_migratable) {
> +    if (only_migratable && dc->vmsd) {
>          if (dc->vmsd->unmigratable) {
>              error_setg(errp, "Device %s is not migratable, but "
>                         "--only-migratable was specified", driver);

This seems like a good fix for the crash as a short term fix,
but longer term I think it would be better to make setting
dc->vmsd mandatory. I think devices which don't set it fall into
one of these categories:
 * deliberately has no VMState struct as it has no state that
   needs migrating -- we should have some kind of flag for
   such devices to positively assert that they don't need to
   deal with migration
 * accidentally failed to provide a VMState struct -- this is
   a bug and the device should be fixed to at minimum mark
   itself as unmigratable (and ideally fixed to implement
   migration!)
 * didn't provide a VMState struct because they handle
   migration manually using vmstate_register() -- we should
   update these to use VMState, or failing that use the
   flag to say "deliberately not providing VMState"

Then we can make QEMU assert if dc->vmsd is NULL, and
catch future occurrences of "oops I didn't think about
migration" bugs in new device models. We also have an
easy way to grep the codebase for devices that need to
have migration implemented.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 January 2017 at 14:41, Ashijeet Acharya <ashijeetacharya@gmail.com> wrote:
>> Commit a3a3d8c7 introduced a segfault bug while checking for
>> 'dc->vmsd->unmigratable' which caused QEMU to crash when trying to add
>> devices which do no set their 'dc->vmsd' yet while initialization.
>> Place a 'dc->vmsd' check prior to it so that we do not segfault for
>> such devices.
>>
>> NOTE: This doesn't compromise the functioning of --only-migratable
>> option as all the unmigratable devices do set their 'dc->vmsd'.
>>
>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> ---
>>  qdev-monitor.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 81d01df..a1106fd 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -578,7 +578,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>          return NULL;
>>      }
>>
>> -    if (only_migratable) {
>> +    if (only_migratable && dc->vmsd) {
>>          if (dc->vmsd->unmigratable) {
>>              error_setg(errp, "Device %s is not migratable, but "
>>                         "--only-migratable was specified", driver);
>
> This seems like a good fix for the crash as a short term fix,
> but longer term I think it would be better to make setting
> dc->vmsd mandatory. I think devices which don't set it fall into
> one of these categories:
>  * deliberately has no VMState struct as it has no state that
>    needs migrating -- we should have some kind of flag for
>    such devices to positively assert that they don't need to
>    deal with migration
>  * accidentally failed to provide a VMState struct -- this is
>    a bug and the device should be fixed to at minimum mark
>    itself as unmigratable (and ideally fixed to implement
>    migration!)
>  * didn't provide a VMState struct because they handle
>    migration manually using vmstate_register() -- we should
>    update these to use VMState, or failing that use the
>    flag to say "deliberately not providing VMState"
>
> Then we can make QEMU assert if dc->vmsd is NULL, and
> catch future occurrences of "oops I didn't think about
> migration" bugs in new device models. We also have an
> easy way to grep the codebase for devices that need to
> have migration implemented.

+1

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable

[Ashijeet Acharya]

On Mon, Jan 30, 2017 at 10:08 PM, Juan Quintela <quintela@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 30 January 2017 at 14:41, Ashijeet Acharya <ashijeetacharya@gmail.com> wrote:
>>> Commit a3a3d8c7 introduced a segfault bug while checking for
>>> 'dc->vmsd->unmigratable' which caused QEMU to crash when trying to add
>>> devices which do no set their 'dc->vmsd' yet while initialization.
>>> Place a 'dc->vmsd' check prior to it so that we do not segfault for
>>> such devices.
>>>
>>> NOTE: This doesn't compromise the functioning of --only-migratable
>>> option as all the unmigratable devices do set their 'dc->vmsd'.
>>>
>>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>>> ---
>>>  qdev-monitor.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>> index 81d01df..a1106fd 100644
>>> --- a/qdev-monitor.c
>>> +++ b/qdev-monitor.c
>>> @@ -578,7 +578,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>>          return NULL;
>>>      }
>>>
>>> -    if (only_migratable) {
>>> +    if (only_migratable && dc->vmsd) {
>>>          if (dc->vmsd->unmigratable) {
>>>              error_setg(errp, "Device %s is not migratable, but "
>>>                         "--only-migratable was specified", driver);
>>
>> This seems like a good fix for the crash as a short term fix,
>> but longer term I think it would be better to make setting
>> dc->vmsd mandatory. I think devices which don't set it fall into
>> one of these categories:
>>  * deliberately has no VMState struct as it has no state that
>>    needs migrating -- we should have some kind of flag for
>>    such devices to positively assert that they don't need to
>>    deal with migration
>>  * accidentally failed to provide a VMState struct -- this is
>>    a bug and the device should be fixed to at minimum mark
>>    itself as unmigratable (and ideally fixed to implement
>>    migration!)
>>  * didn't provide a VMState struct because they handle
>>    migration manually using vmstate_register() -- we should
>>    update these to use VMState, or failing that use the
>>    flag to say "deliberately not providing VMState"
>>
>> Then we can make QEMU assert if dc->vmsd is NULL, and
>> catch future occurrences of "oops I didn't think about
>> migration" bugs in new device models. We also have an
>> easy way to grep the codebase for devices that need to
>> have migration implemented.
>
> +1
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>

I just realised that I had to make this fix at two different places
i.e. inside usbdevice_create() in hw/usb/bus.c as well. I will send a
v2 for that and already include Juan's R-b tag.

Ashijeet