[Qemu-devel] [PATCH v2 0/2] virtio-scsi: fix hotplug ->reset() vs event race

[Qemu-devel] [PATCH v2 0/2] virtio-scsi: fix hotplug ->reset() vs event race

[Stefan Hajnoczi]

v2:
 * Drop Error **errp argument to post_plug() handler [Paolo]
 * Move post_plug() call outside if (dev->hotplugged)

The virtio-scsi command virtqueues run during hotplug.  This creates the
possibility of race conditions since the guest can submit commands while the
monitor is performing hotplug.

See Patch 2 for a fix for the ->reset() vs event race condition that Zhengui Li
encountered.

Stefan Hajnoczi (2):
  qdev: add HotplugHandler->post_plug() callback
  virtio-scsi: fix hotplug ->reset() vs event race

 include/hw/hotplug.h  | 11 +++++++++++
 hw/core/hotplug.c     | 10 ++++++++++
 hw/core/qdev.c        |  4 ++++
 hw/scsi/virtio-scsi.c | 11 ++++++++++-
 4 files changed, 35 insertions(+), 1 deletion(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/2] qdev: add HotplugHandler->post_plug() callback

[Stefan Hajnoczi]

The ->pre_plug() callback is invoked before the device is realized.  The
->plug() callback is invoked when the device is being realized but
before it is reset.

This patch adds a ->post_plug() callback which is invoked after the
device has been reset.  This callback is needed by HotplugHandlers that
need to wait until after ->reset().

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/hotplug.h | 11 +++++++++++
 hw/core/hotplug.c    | 10 ++++++++++
 hw/core/qdev.c       |  4 ++++
 3 files changed, 25 insertions(+)

diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
index 1a0516a479..51541d63e1 100644
--- a/include/hw/hotplug.h
+++ b/include/hw/hotplug.h
@@ -47,6 +47,8 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
  * @parent: Opaque parent interface.
  * @pre_plug: pre plug callback called at start of device.realize(true)
  * @plug: plug callback called at end of device.realize(true).
+ * @post_plug: post plug callback called after device.realize(true) and device
+ *             reset
  * @unplug_request: unplug request callback.
  *                  Used as a means to initiate device unplug for devices that
  *                  require asynchronous unplug handling.
@@ -61,6 +63,7 @@ typedef struct HotplugHandlerClass {
     /* <public> */
     hotplug_fn pre_plug;
     hotplug_fn plug;
+    void (*post_plug)(HotplugHandler *plug_handler, DeviceState *plugged_dev);
     hotplug_fn unplug_request;
     hotplug_fn unplug;
 } HotplugHandlerClass;
@@ -83,6 +86,14 @@ void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
                               DeviceState *plugged_dev,
                               Error **errp);
 
+/**
+ * hotplug_handler_post_plug:
+ *
+ * Call #HotplugHandlerClass.post_plug callback of @plug_handler.
+ */
+void hotplug_handler_post_plug(HotplugHandler *plug_handler,
+                               DeviceState *plugged_dev);
+
 /**
  * hotplug_handler_unplug_request:
  *
diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
index 17ac986685..2253072d0e 100644
--- a/hw/core/hotplug.c
+++ b/hw/core/hotplug.c
@@ -35,6 +35,16 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
     }
 }
 
+void hotplug_handler_post_plug(HotplugHandler *plug_handler,
+                               DeviceState *plugged_dev)
+{
+    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
+
+    if (hdc->post_plug) {
+        hdc->post_plug(plug_handler, plugged_dev);
+    }
+}
+
 void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
                                     DeviceState *plugged_dev,
                                     Error **errp)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cf0db4b6da..529b82de18 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -867,6 +867,10 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             device_reset(dev);
         }
         dev->pending_deleted_event = false;
+
+        if (hotplug_ctrl) {
+            hotplug_handler_post_plug(hotplug_ctrl, dev);
+        }
     } else if (!value && dev->realized) {
         Error **local_errp = NULL;
         QLIST_FOREACH(bus, &dev->child_bus, sibling) {
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/2] virtio-scsi: fix hotplug ->reset() vs event race

[Stefan Hajnoczi]

There is a race condition during hotplug when iothread is used.  It
occurs because virtio-scsi may be processing command queues in the
iothread while the monitor performs SCSI device hotplug.

When a SCSI device is hotplugged the HotplugHandler->plug() callback is
invoked and virtio-scsi emits a rescan event to the guest.

If the guest submits a SCSI command at this point then it may be
cancelled before hotplug completes.  This happens because ->reset() is
called by hw/core/qdev.c:device_set_realized() after
HotplugHandler->plug() has been called and
hw/scsi/scsi-disk.c:scsi_disk_reset() purges all requests.

This patch uses the new HotplugHandler->post_plug() callback to emit the
rescan event after ->reset().  This eliminates the race conditions where
requests could be cancelled.

Reported-by: l00284672 <lizhengui@huawei.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Fam Zheng <famz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/scsi/virtio-scsi.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 3aa99717e2..5a3057d1f8 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -797,8 +797,16 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
         virtio_scsi_acquire(s);
         blk_set_aio_context(sd->conf.blk, s->ctx);
         virtio_scsi_release(s);
-
     }
+}
+
+/* Announce the new device after it has been plugged */
+static void virtio_scsi_post_hotplug(HotplugHandler *hotplug_dev,
+                                     DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev);
+    VirtIOSCSI *s = VIRTIO_SCSI(vdev);
+    SCSIDevice *sd = SCSI_DEVICE(dev);
 
     if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
         virtio_scsi_acquire(s);
@@ -968,6 +976,7 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data)
     vdc->start_ioeventfd = virtio_scsi_dataplane_start;
     vdc->stop_ioeventfd = virtio_scsi_dataplane_stop;
     hc->plug = virtio_scsi_hotplug;
+    hc->post_plug = virtio_scsi_post_hotplug;
     hc->unplug = virtio_scsi_hotunplug;
 }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 0/2] virtio-scsi: fix hotplug ->reset() vs event race

[Igor Mammedov]

On Mon, 16 Jul 2018 09:37:30 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> v2:
>  * Drop Error **errp argument to post_plug() handler [Paolo]
>  * Move post_plug() call outside if (dev->hotplugged)
> 
> The virtio-scsi command virtqueues run during hotplug.  This creates the
> possibility of race conditions since the guest can submit commands while the
> monitor is performing hotplug.
> 
> See Patch 2 for a fix for the ->reset() vs event race condition that Zhengui Li
> encountered.
> 
> Stefan Hajnoczi (2):
>   qdev: add HotplugHandler->post_plug() callback
>   virtio-scsi: fix hotplug ->reset() vs event race
> 
>  include/hw/hotplug.h  | 11 +++++++++++
>  hw/core/hotplug.c     | 10 ++++++++++
>  hw/core/qdev.c        |  4 ++++
>  hw/scsi/virtio-scsi.c | 11 ++++++++++-
>  4 files changed, 35 insertions(+), 1 deletion(-)
> 

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/2] virtio-scsi: fix hotplug ->reset() vs event race

[Michael S. Tsirkin]

On Mon, Jul 16, 2018 at 09:37:32AM +0100, Stefan Hajnoczi wrote:
> There is a race condition during hotplug when iothread is used.  It
> occurs because virtio-scsi may be processing command queues in the
> iothread while the monitor performs SCSI device hotplug.
> 
> When a SCSI device is hotplugged the HotplugHandler->plug() callback is
> invoked and virtio-scsi emits a rescan event to the guest.
> 
> If the guest submits a SCSI command at this point then it may be
> cancelled before hotplug completes.  This happens because ->reset() is
> called by hw/core/qdev.c:device_set_realized() after
> HotplugHandler->plug() has been called and
> hw/scsi/scsi-disk.c:scsi_disk_reset() purges all requests.
> 
> This patch uses the new HotplugHandler->post_plug() callback to emit the
> rescan event after ->reset().  This eliminates the race conditions where
> requests could be cancelled.
> 
> Reported-by: l00284672 <lizhengui@huawei.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Fam Zheng <famz@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

Pls merge through scsi tree.

> ---
>  hw/scsi/virtio-scsi.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 3aa99717e2..5a3057d1f8 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -797,8 +797,16 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          virtio_scsi_acquire(s);
>          blk_set_aio_context(sd->conf.blk, s->ctx);
>          virtio_scsi_release(s);
> -
>      }
> +}
> +
> +/* Announce the new device after it has been plugged */
> +static void virtio_scsi_post_hotplug(HotplugHandler *hotplug_dev,
> +                                     DeviceState *dev)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev);
> +    VirtIOSCSI *s = VIRTIO_SCSI(vdev);
> +    SCSIDevice *sd = SCSI_DEVICE(dev);
>  
>      if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
>          virtio_scsi_acquire(s);
> @@ -968,6 +976,7 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data)
>      vdc->start_ioeventfd = virtio_scsi_dataplane_start;
>      vdc->stop_ioeventfd = virtio_scsi_dataplane_stop;
>      hc->plug = virtio_scsi_hotplug;
> +    hc->post_plug = virtio_scsi_post_hotplug;
>      hc->unplug = virtio_scsi_hotunplug;
>  }
>  
> -- 
> 2.17.1

Re: [Qemu-devel] [PATCH v2 2/2] virtio-scsi: fix hotplug ->reset() vs event race

[Paolo Bonzini]

On 16/07/2018 14:40, Michael S. Tsirkin wrote:
> On Mon, Jul 16, 2018 at 09:37:32AM +0100, Stefan Hajnoczi wrote:
>> There is a race condition during hotplug when iothread is used.  It
>> occurs because virtio-scsi may be processing command queues in the
>> iothread while the monitor performs SCSI device hotplug.
>>
>> When a SCSI device is hotplugged the HotplugHandler->plug() callback is
>> invoked and virtio-scsi emits a rescan event to the guest.
>>
>> If the guest submits a SCSI command at this point then it may be
>> cancelled before hotplug completes.  This happens because ->reset() is
>> called by hw/core/qdev.c:device_set_realized() after
>> HotplugHandler->plug() has been called and
>> hw/scsi/scsi-disk.c:scsi_disk_reset() purges all requests.
>>
>> This patch uses the new HotplugHandler->post_plug() callback to emit the
>> rescan event after ->reset().  This eliminates the race conditions where
>> requests could be cancelled.
>>
>> Reported-by: l00284672 <lizhengui@huawei.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Fam Zheng <famz@redhat.com>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Pls merge through scsi tree.

Will do, thanks Stefan.

Paolo

> 
>> ---
>>  hw/scsi/virtio-scsi.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>> index 3aa99717e2..5a3057d1f8 100644
>> --- a/hw/scsi/virtio-scsi.c
>> +++ b/hw/scsi/virtio-scsi.c
>> @@ -797,8 +797,16 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>          virtio_scsi_acquire(s);
>>          blk_set_aio_context(sd->conf.blk, s->ctx);
>>          virtio_scsi_release(s);
>> -
>>      }
>> +}
>> +
>> +/* Announce the new device after it has been plugged */
>> +static void virtio_scsi_post_hotplug(HotplugHandler *hotplug_dev,
>> +                                     DeviceState *dev)
>> +{
>> +    VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev);
>> +    VirtIOSCSI *s = VIRTIO_SCSI(vdev);
>> +    SCSIDevice *sd = SCSI_DEVICE(dev);
>>  
>>      if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
>>          virtio_scsi_acquire(s);
>> @@ -968,6 +976,7 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data)
>>      vdc->start_ioeventfd = virtio_scsi_dataplane_start;
>>      vdc->stop_ioeventfd = virtio_scsi_dataplane_stop;
>>      hc->plug = virtio_scsi_hotplug;
>> +    hc->post_plug = virtio_scsi_post_hotplug;
>>      hc->unplug = virtio_scsi_hotunplug;
>>  }
>>  
>> -- 
>> 2.17.1