[Qemu-devel] [PATCH for-2.9] virtio-scsi: Fix assertion failure on dataplane handlers

[Qemu-devel] [PATCH for-2.9] virtio-scsi: Fix assertion failure on dataplane handlers

[Fam Zheng]

After the AioContext lock push down, there is a race between
virtio_scsi_dataplane_start and those "assert(s->ctx &&
s->dataplane_started)", because the latter doesn't isn't wrapped in
aio_context_acquire.

Reproducer is simply booting a Fedora guest with an empty
virtio-scsi-dataplane controller:

    qemu-system-x86_64 \
      -drive if=none,id=root,format=raw,file=Fedora-Cloud-Base-25-1.3.x86_64.raw \
      -device virtio-scsi \
      -device scsi-disk,drive=root,bootindex=1 \
      -object iothread,id=io \
      -device virtio-scsi-pci,iothread=io \
      -net user,hostfwd=tcp::10022-:22 -net nic,model=virtio -m 2048 \
      --enable-kvm

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/virtio-scsi-dataplane.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 74c95e0..12b44b7 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -52,28 +52,40 @@ void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp)
 static bool virtio_scsi_data_plane_handle_cmd(VirtIODevice *vdev,
                                               VirtQueue *vq)
 {
-    VirtIOSCSI *s = (VirtIOSCSI *)vdev;
+    bool progress;
+    VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
+    aio_context_acquire(s->ctx);
     assert(s->ctx && s->dataplane_started);
-    return virtio_scsi_handle_cmd_vq(s, vq);
+    progress = virtio_scsi_handle_cmd_vq(s, vq);
+    aio_context_release(s->ctx);
+    return progress;
 }
 
 static bool virtio_scsi_data_plane_handle_ctrl(VirtIODevice *vdev,
                                                VirtQueue *vq)
 {
+    bool progress;
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
+    aio_context_acquire(s->ctx);
     assert(s->ctx && s->dataplane_started);
-    return virtio_scsi_handle_ctrl_vq(s, vq);
+    progress = virtio_scsi_handle_ctrl_vq(s, vq);
+    aio_context_release(s->ctx);
+    return progress;
 }
 
 static bool virtio_scsi_data_plane_handle_event(VirtIODevice *vdev,
                                                 VirtQueue *vq)
 {
+    bool progress;
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
+    aio_context_acquire(s->ctx);
     assert(s->ctx && s->dataplane_started);
-    return virtio_scsi_handle_event_vq(s, vq);
+    progress = virtio_scsi_handle_event_vq(s, vq);
+    aio_context_release(s->ctx);
+    return progress;
 }
 
 static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n,
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-2.9] virtio-scsi: Fix assertion failure on dataplane handlers

[Paolo Bonzini]

On 14/03/2017 14:26, Fam Zheng wrote:
> After the AioContext lock push down, there is a race between
> virtio_scsi_dataplane_start and those "assert(s->ctx &&
> s->dataplane_started)", because the latter doesn't isn't wrapped in
> aio_context_acquire.
> 
> Reproducer is simply booting a Fedora guest with an empty
> virtio-scsi-dataplane controller:
> 
>     qemu-system-x86_64 \
>       -drive if=none,id=root,format=raw,file=Fedora-Cloud-Base-25-1.3.x86_64.raw \
>       -device virtio-scsi \
>       -device scsi-disk,drive=root,bootindex=1 \
>       -object iothread,id=io \
>       -device virtio-scsi-pci,iothread=io \
>       -net user,hostfwd=tcp::10022-:22 -net nic,model=virtio -m 2048 \
>       --enable-kvm
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/scsi/virtio-scsi-dataplane.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index 74c95e0..12b44b7 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -52,28 +52,40 @@ void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp)
>  static bool virtio_scsi_data_plane_handle_cmd(VirtIODevice *vdev,
>                                                VirtQueue *vq)
>  {
> -    VirtIOSCSI *s = (VirtIOSCSI *)vdev;
> +    bool progress;
> +    VirtIOSCSI *s = VIRTIO_SCSI(vdev);
>  
> +    aio_context_acquire(s->ctx);
>      assert(s->ctx && s->dataplane_started);
> -    return virtio_scsi_handle_cmd_vq(s, vq);
> +    progress = virtio_scsi_handle_cmd_vq(s, vq);
> +    aio_context_release(s->ctx);

Can you use virtio_scsi_acquire/release, and remove it from
virtio_scsi_handle_*_vq?

Paolo

> +    return progress;
>  }
>  
>  static bool virtio_scsi_data_plane_handle_ctrl(VirtIODevice *vdev,
>                                                 VirtQueue *vq)
>  {
> +    bool progress;
>      VirtIOSCSI *s = VIRTIO_SCSI(vdev);
>  
> +    aio_context_acquire(s->ctx);
>      assert(s->ctx && s->dataplane_started);
> -    return virtio_scsi_handle_ctrl_vq(s, vq);
> +    progress = virtio_scsi_handle_ctrl_vq(s, vq);
> +    aio_context_release(s->ctx);
> +    return progress;
>  }
>  
>  static bool virtio_scsi_data_plane_handle_event(VirtIODevice *vdev,
>                                                  VirtQueue *vq)
>  {
> +    bool progress;
>      VirtIOSCSI *s = VIRTIO_SCSI(vdev);
>  
> +    aio_context_acquire(s->ctx);
>      assert(s->ctx && s->dataplane_started);
> -    return virtio_scsi_handle_event_vq(s, vq);
> +    progress = virtio_scsi_handle_event_vq(s, vq);
> +    aio_context_release(s->ctx);
> +    return progress;
>  }
>  
>  static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n,
>