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

Ashijeet Acharya posted 1 patch 7 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1486198784-31523-1-git-send-email-ashijeetacharya@gmail.com
Test checkpatch passed
Test docker passed
Test s390x failed
There is a newer version of this series
hw/usb/bus.c   | 2 +-
qdev-monitor.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
[Qemu-devel] [PATCH v2] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable
Posted by Ashijeet Acharya 7 years, 2 months ago
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>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
Changes in v2:
- place dc->vmsd check in hw/usb/bus.c as well
---
 hw/usb/bus.c   | 2 +-
 qdev-monitor.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 1dcc35c..1e39b2c 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -731,7 +731,7 @@ USBDevice *usbdevice_create(const char *cmdline)
 
     dc = DEVICE_CLASS(klass);
 
-    if (only_migratable) {
+    if (only_migratable && dc->vmsd) {
         if (dc->vmsd->unmigratable) {
             error_report("Device %s is not migratable, but --only-migratable "
                          "was specified", f->name);
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 549f45f..b72e5a4 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -579,7 +579,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 v2] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable
Posted by Paolo Bonzini 7 years, 2 months ago

On 04/02/2017 00:59, Ashijeet Acharya 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>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> ---
> Changes in v2:
> - place dc->vmsd check in hw/usb/bus.c as well
> ---
>  hw/usb/bus.c   | 2 +-
>  qdev-monitor.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index 1dcc35c..1e39b2c 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -731,7 +731,7 @@ USBDevice *usbdevice_create(const char *cmdline)
>  
>      dc = DEVICE_CLASS(klass);
>  
> -    if (only_migratable) {
> +    if (only_migratable && dc->vmsd) {
>          if (dc->vmsd->unmigratable) {
>              error_report("Device %s is not migratable, but --only-migratable "
>                           "was specified", f->name);
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 549f45f..b72e5a4 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -579,7 +579,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);
> 

Could you handle only_migratable in device_set_realized (inside "if
(value && !dev->realized)"), to avoid the code duplication?  Separate
patch from this one, of course.

Thanks,

Paolo