[Qemu-devel] [PATCH for-2.11 v3 0/2] Check for the availability of a hotplug controller before adding a device

[Qemu-devel] [PATCH for-2.11 v3 0/2] Check for the availability of a hotplug controller before adding a device

[Thomas Huth]

First patch is a small clean up to the error handling code in
qdev_device_add(), and the second patch adds a proper check for
the availability of a hotplug controller to prevent the possibility
of a crash with device_del.

The crash can currently be triggered for example like this:

$ s390x-softmmu/qemu-system-s390x -M none -nographic 
QEMU 2.10.50 monitor - type 'help' for more information
(qemu) device_add qemu-s390x-cpu,id=x
(qemu) device_del x
**
ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
Aborted (core dumped)

v3:
 - Invert the logic of the second error check in the first patch
   as suggested by Igor
 - Updated the patch description of the second patch with the current
   way to crash QEMU

v2:
 - Do the check for the availability of the hotplug controller earlier
   in qdev_device_add
 - Use common new err_dev_del handler in case of failure

Thomas Huth (2):
  qdev_monitor: Simplify error handling in qdev_device_add()
  qdev: Check for the availability of a hotplug controller before adding
    a device

 hw/core/qdev.c         | 28 ++++++++++++++++++++--------
 include/hw/qdev-core.h |  1 +
 qdev-monitor.c         | 21 +++++++++++++--------
 3 files changed, 34 insertions(+), 16 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/2] qdev_monitor: Simplify error handling in qdev_device_add()

[Thomas Huth]

Instead of doing the clean-ups on errors multiple times, introduce
a jump label at the end of the function that can be used by all
error paths that need this cleanup.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 qdev-monitor.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 8fd6df9..9188d20 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -620,22 +620,22 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 
     /* set properties */
     if (qemu_opt_foreach(opts, set_property, dev, &err)) {
-        error_propagate(errp, err);
-        object_unparent(OBJECT(dev));
-        object_unref(OBJECT(dev));
-        return NULL;
+        goto err_del_dev;
     }
 
     dev->opts = opts;
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
     if (err != NULL) {
-        error_propagate(errp, err);
         dev->opts = NULL;
-        object_unparent(OBJECT(dev));
-        object_unref(OBJECT(dev));
-        return NULL;
+        goto err_del_dev;
     }
     return dev;
+
+err_del_dev:
+    error_propagate(errp, err);
+    object_unparent(OBJECT(dev));
+    object_unref(OBJECT(dev));
+    return NULL;
 }
 
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/2] qdev: Check for the availability of a hotplug controller before adding a device

[Thomas Huth]

The qdev_unplug() function contains a g_assert(hotplug_ctrl) statement,
so QEMU crashes when the user tries to device_add + device_del a device
that does not have a corresponding hotplug controller. This could be
provoked for a couple of devices in the past (see commit 4c93950659487c7ad
or 84ebd3e8c7d4fe955 for example), and can currently for example also be
triggered like this:

$ s390x-softmmu/qemu-system-s390x -M none -nographic 
QEMU 2.10.50 monitor - type 'help' for more information
(qemu) device_add qemu-s390x-cpu,id=x
(qemu) device_del x
**
ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
Aborted (core dumped)

So devices clearly need a hotplug controller when they should be usable
with device_add.
The code in qdev_device_add() already checks whether the bus has a proper
hotplug controller, but for devices that do not have a corresponding bus,
there is no appropriate check available yet. In that case we should check
whether the machine itself provides a suitable hotplug controller and
refuse to plug the device if none is available.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/core/qdev.c         | 28 ++++++++++++++++++++--------
 include/hw/qdev-core.h |  1 +
 qdev-monitor.c         |  5 +++++
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 1111295..f739753 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -253,19 +253,31 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
     dev->alias_required_for_version = required_for_version;
 }
 
+HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
+{
+    MachineState *machine;
+    MachineClass *mc;
+    Object *m_obj = qdev_get_machine();
+
+    if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
+        machine = MACHINE(m_obj);
+        mc = MACHINE_GET_CLASS(machine);
+        if (mc->get_hotplug_handler) {
+            return mc->get_hotplug_handler(machine, dev);
+        }
+    }
+
+    return NULL;
+}
+
 HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
 {
-    HotplugHandler *hotplug_ctrl = NULL;
+    HotplugHandler *hotplug_ctrl;
 
     if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
         hotplug_ctrl = dev->parent_bus->hotplug_handler;
-    } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
-        MachineState *machine = MACHINE(qdev_get_machine());
-        MachineClass *mc = MACHINE_GET_CLASS(machine);
-
-        if (mc->get_hotplug_handler) {
-            hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
-        }
+    } else {
+        hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
     }
     return hotplug_ctrl;
 }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 0a71bf8..51473ee 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -286,6 +286,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *name);
 void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
+HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
 HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
 void qdev_unplug(DeviceState *dev, Error **errp);
 void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 9188d20..38c0fc2 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -614,6 +614,11 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 
     if (bus) {
         qdev_set_parent_bus(dev, bus);
+    } else if (qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) {
+        /* No bus, no machine hotplug handler --> device is not hotpluggable */
+        error_setg(&err, "Device '%s' can not be hotplugged on this machine",
+                   driver);
+        goto err_del_dev;
     }
 
     qdev_set_id(dev, qemu_opts_id(opts));
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 1/2] qdev_monitor: Simplify error handling in qdev_device_add()

[Cornelia Huck]

On Thu,  2 Nov 2017 11:10:05 +0100
Thomas Huth <thuth@redhat.com> wrote:

> Instead of doing the clean-ups on errors multiple times, introduce
> a jump label at the end of the function that can be used by all
> error paths that need this cleanup.
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  qdev-monitor.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH v3 2/2] qdev: Check for the availability of a hotplug controller before adding a device

[Cornelia Huck]

On Thu,  2 Nov 2017 11:10:06 +0100
Thomas Huth <thuth@redhat.com> wrote:

> The qdev_unplug() function contains a g_assert(hotplug_ctrl) statement,
> so QEMU crashes when the user tries to device_add + device_del a device
> that does not have a corresponding hotplug controller. This could be
> provoked for a couple of devices in the past (see commit 4c93950659487c7ad
> or 84ebd3e8c7d4fe955 for example), and can currently for example also be
> triggered like this:
> 
> $ s390x-softmmu/qemu-system-s390x -M none -nographic 
> QEMU 2.10.50 monitor - type 'help' for more information
> (qemu) device_add qemu-s390x-cpu,id=x
> (qemu) device_del x
> **
> ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> Aborted (core dumped)
> 
> So devices clearly need a hotplug controller when they should be usable
> with device_add.
> The code in qdev_device_add() already checks whether the bus has a proper
> hotplug controller, but for devices that do not have a corresponding bus,
> there is no appropriate check available yet. In that case we should check
> whether the machine itself provides a suitable hotplug controller and
> refuse to plug the device if none is available.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/core/qdev.c         | 28 ++++++++++++++++++++--------
>  include/hw/qdev-core.h |  1 +
>  qdev-monitor.c         |  5 +++++
>  3 files changed, 26 insertions(+), 8 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH v3 2/2] qdev: Check for the availability of a hotplug controller before adding a device

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> The qdev_unplug() function contains a g_assert(hotplug_ctrl) statement,
> so QEMU crashes when the user tries to device_add + device_del a device
> that does not have a corresponding hotplug controller. This could be
> provoked for a couple of devices in the past (see commit 4c93950659487c7ad
> or 84ebd3e8c7d4fe955 for example), and can currently for example also be
> triggered like this:
>
> $ s390x-softmmu/qemu-system-s390x -M none -nographic 
> QEMU 2.10.50 monitor - type 'help' for more information
> (qemu) device_add qemu-s390x-cpu,id=x
> (qemu) device_del x
> **
> ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> Aborted (core dumped)
>
> So devices clearly need a hotplug controller when they should be usable
> with device_add.
> The code in qdev_device_add() already checks whether the bus has a proper
> hotplug controller,

Where?  Hmm, I guess it's this one:

    if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
        error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
        return NULL;
    }

>                     but for devices that do not have a corresponding bus,
> there is no appropriate check available yet. In that case we should check
> whether the machine itself provides a suitable hotplug controller and
> refuse to plug the device if none is available.
>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/core/qdev.c         | 28 ++++++++++++++++++++--------
>  include/hw/qdev-core.h |  1 +
>  qdev-monitor.c         |  5 +++++
>  3 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 1111295..f739753 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -253,19 +253,31 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>      dev->alias_required_for_version = required_for_version;
>  }
>  
> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
> +{
> +    MachineState *machine;
> +    MachineClass *mc;
> +    Object *m_obj = qdev_get_machine();
> +
> +    if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
> +        machine = MACHINE(m_obj);
> +        mc = MACHINE_GET_CLASS(machine);
> +        if (mc->get_hotplug_handler) {
> +            return mc->get_hotplug_handler(machine, dev);
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
>  HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
>  {
> -    HotplugHandler *hotplug_ctrl = NULL;
> +    HotplugHandler *hotplug_ctrl;
>  
>      if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>          hotplug_ctrl = dev->parent_bus->hotplug_handler;
> -    } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
> -        MachineState *machine = MACHINE(qdev_get_machine());
> -        MachineClass *mc = MACHINE_GET_CLASS(machine);
> -
> -        if (mc->get_hotplug_handler) {
> -            hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> -        }
> +    } else {
> +        hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
>      }
>      return hotplug_ctrl;
>  }

qdev_get_machine_hotplug_handler() factored out of
qdev_get_hotplug_handler().  Okay.  Announcing it in the commit message
could've saved me a few review brainwaves.

> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 0a71bf8..51473ee 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -286,6 +286,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *name);
>  void qdev_init_nofail(DeviceState *dev);
>  void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>                                   int required_for_version);
> +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
>  HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
>  void qdev_unplug(DeviceState *dev, Error **errp);
>  void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 9188d20..38c0fc2 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -614,6 +614,11 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>  
       if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
           error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
           return NULL;
       }

       if (!migration_is_idle()) {
           error_setg(errp, "device_add not allowed while migrating");
           return NULL;
       }

       /* create device */
       dev = DEVICE(object_new(driver));

>      if (bus) {
>          qdev_set_parent_bus(dev, bus);
> +    } else if (qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) {
> +        /* No bus, no machine hotplug handler --> device is not hotpluggable */

Long line.

> +        error_setg(&err, "Device '%s' can not be hotplugged on this machine",
> +                   driver);
> +        goto err_del_dev;
>      }
>  
>      qdev_set_id(dev, qemu_opts_id(opts));

Hmm.  We need to check "can hotplug" in two separate ways, with bus and
without bus.  Can we keep the two ways on one place?  Something like

       if (qdev_hotplug) {
           if (bus && !qbus_is_hotpluggable(bus)) {
               error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
               return NULL;
           }
           if (!bus && !qdev_get_machine_hotplug_handler(dev)) {
               error_setg(&err,
                          "Machine doesn't support hot-plugging device '%s'"
                          driver);
               return NULL;
           }
      }

Re: [Qemu-devel] [PATCH v3 2/2] qdev: Check for the availability of a hotplug controller before adding a device

[Thomas Huth]

On 13.11.2017 15:00, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> The qdev_unplug() function contains a g_assert(hotplug_ctrl) statement,
>> so QEMU crashes when the user tries to device_add + device_del a device
>> that does not have a corresponding hotplug controller. This could be
>> provoked for a couple of devices in the past (see commit 4c93950659487c7ad
>> or 84ebd3e8c7d4fe955 for example), and can currently for example also be
>> triggered like this:
>>
>> $ s390x-softmmu/qemu-system-s390x -M none -nographic 
>> QEMU 2.10.50 monitor - type 'help' for more information
>> (qemu) device_add qemu-s390x-cpu,id=x
>> (qemu) device_del x
>> **
>> ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
>> Aborted (core dumped)
>>
>> So devices clearly need a hotplug controller when they should be usable
>> with device_add.
>> The code in qdev_device_add() already checks whether the bus has a proper
>> hotplug controller,
> 
> Where?  Hmm, I guess it's this one:
> 
>     if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
>         error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
>         return NULL;
>     }

Right.

>>                     but for devices that do not have a corresponding bus,
>> there is no appropriate check available yet. In that case we should check
>> whether the machine itself provides a suitable hotplug controller and
>> refuse to plug the device if none is available.
[...]
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 9188d20..38c0fc2 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -614,6 +614,11 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>  
>        if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
>            error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
>            return NULL;
>        }
> 
>        if (!migration_is_idle()) {
>            error_setg(errp, "device_add not allowed while migrating");
>            return NULL;
>        }
> 
>        /* create device */
>        dev = DEVICE(object_new(driver));
> 
>>      if (bus) {
>>          qdev_set_parent_bus(dev, bus);
>> +    } else if (qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) {
>> +        /* No bus, no machine hotplug handler --> device is not hotpluggable */
> 
> Long line.

Less than 80 columns, so that's fine.

>> +        error_setg(&err, "Device '%s' can not be hotplugged on this machine",
>> +                   driver);
>> +        goto err_del_dev;
>>      }
>>  
>>      qdev_set_id(dev, qemu_opts_id(opts));
> 
> Hmm.  We need to check "can hotplug" in two separate ways, with bus and
> without bus.  Can we keep the two ways on one place?  Something like
> 
>        if (qdev_hotplug) {
>            if (bus && !qbus_is_hotpluggable(bus)) {
>                error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
>                return NULL;
>            }
>            if (!bus && !qdev_get_machine_hotplug_handler(dev)) {
>                error_setg(&err,
>                           "Machine doesn't support hot-plugging device '%s'"
>                           driver);
>                return NULL;
>            }
>       }

We likely could ... but I don't like that change since in that case
you've got to always do "dev = DEVICE(object_new(driver))" (and the code
needs to be at the end of the function, so it would need "goto
err_del_dev" instead of "return NULL"). If we keep the checks separate,
the first check can bail out without creating the device first, so I'd
really prefer to keep my patch that way.

 Thomas

Re: [Qemu-devel] [PATCH for-2.11 v3 0/2] Check for the availability of a hotplug controller before adding a device

[Thomas Huth]

On 02.11.2017 11:10, Thomas Huth wrote:
> First patch is a small clean up to the error handling code in
> qdev_device_add(), and the second patch adds a proper check for
> the availability of a hotplug controller to prevent the possibility
> of a crash with device_del.
> 
> The crash can currently be triggered for example like this:
> 
> $ s390x-softmmu/qemu-system-s390x -M none -nographic 
> QEMU 2.10.50 monitor - type 'help' for more information
> (qemu) device_add qemu-s390x-cpu,id=x
> (qemu) device_del x
> **
> ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> Aborted (core dumped)
> 
> v3:
>  - Invert the logic of the second error check in the first patch
>    as suggested by Igor
>  - Updated the patch description of the second patch with the current
>    way to crash QEMU
> 
> v2:
>  - Do the check for the availability of the hotplug controller earlier
>    in qdev_device_add
>  - Use common new err_dev_del handler in case of failure
> 
> Thomas Huth (2):
>   qdev_monitor: Simplify error handling in qdev_device_add()
>   qdev: Check for the availability of a hotplug controller before adding
>     a device
> 
>  hw/core/qdev.c         | 28 ++++++++++++++++++++--------
>  include/hw/qdev-core.h |  1 +
>  qdev-monitor.c         | 21 +++++++++++++--------
>  3 files changed, 34 insertions(+), 16 deletions(-)

Ping!

Paolo or Eduardo, could one of you please pick up the two patches?

 Thanks,
  Thomas

Re: [Qemu-devel] [PATCH for-2.11 v3 0/2] Check for the availability of a hotplug controller before adding a device

[Eduardo Habkost]

On Mon, Dec 18, 2017 at 12:59:50PM +0100, Thomas Huth wrote:
> On 02.11.2017 11:10, Thomas Huth wrote:
> > First patch is a small clean up to the error handling code in
> > qdev_device_add(), and the second patch adds a proper check for
> > the availability of a hotplug controller to prevent the possibility
> > of a crash with device_del.
> > 
> > The crash can currently be triggered for example like this:
> > 
> > $ s390x-softmmu/qemu-system-s390x -M none -nographic 
> > QEMU 2.10.50 monitor - type 'help' for more information
> > (qemu) device_add qemu-s390x-cpu,id=x
> > (qemu) device_del x
> > **
> > ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> > Aborted (core dumped)
> > 
> > v3:
> >  - Invert the logic of the second error check in the first patch
> >    as suggested by Igor
> >  - Updated the patch description of the second patch with the current
> >    way to crash QEMU
> > 
> > v2:
> >  - Do the check for the availability of the hotplug controller earlier
> >    in qdev_device_add
> >  - Use common new err_dev_del handler in case of failure
> > 
> > Thomas Huth (2):
> >   qdev_monitor: Simplify error handling in qdev_device_add()
> >   qdev: Check for the availability of a hotplug controller before adding
> >     a device
> > 
> >  hw/core/qdev.c         | 28 ++++++++++++++++++++--------
> >  include/hw/qdev-core.h |  1 +
> >  qdev-monitor.c         | 21 +++++++++++++--------
> >  3 files changed, 34 insertions(+), 16 deletions(-)
> 
> Ping!
> 
> Paolo or Eduardo, could one of you please pick up the two patches?

Very sorry for the long delay.  I just queued it on machine-next.
Thanks!

-- 
Eduardo