[2/6] block: Acquire the AioContext in scsi_*_realize()

Commit Message

Alberto Garcia Jan. 10, 2019, 3:03 p.m. UTC

This fixes the following crash:

{ "execute": "blockdev-add",
  "arguments": {"driver": "null-co", "node-name": "hd0"}}
{ "execute": "object-add",
  "arguments": {"qom-type": "iothread", "id": "iothread0"}}
{ "execute": "x-blockdev-set-iothread",
  "arguments": {"node-name": "hd0", "iothread": "iothread0"}}
{ "execute": "device_add",
   "arguments": {"id": "scsi-pci0", "driver": "virtio-scsi-pci"}}
{ "execute": "device_add",
  "arguments": {"id": "scsi-hd0", "driver": "scsi-hd", "drive": "hd0"}}
qemu: qemu_mutex_unlock_impl: Operation not permitted
Aborted

Signed-off-by: Alberto Garcia <[e-mail]>
---
 hw/scsi/scsi-disk.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

Comments

Kevin Wolf Jan. 11, 2019, 3:02 p.m. UTC | #1

Am 10.01.2019 um 16:03 hat Alberto Garcia geschrieben:
> This fixes the following crash:
> 
> { "execute": "blockdev-add",
>   "arguments": {"driver": "null-co", "node-name": "hd0"}}
> { "execute": "object-add",
>   "arguments": {"qom-type": "iothread", "id": "iothread0"}}
> { "execute": "x-blockdev-set-iothread",
>   "arguments": {"node-name": "hd0", "iothread": "iothread0"}}
> { "execute": "device_add",
>    "arguments": {"id": "scsi-pci0", "driver": "virtio-scsi-pci"}}
> { "execute": "device_add",
>   "arguments": {"id": "scsi-hd0", "driver": "scsi-hd", "drive": "hd0"}}
> qemu: qemu_mutex_unlock_impl: Operation not permitted
> Aborted
> 
> Signed-off-by: Alberto Garcia <[e-mail]>

> @@ -2553,6 +2563,7 @@ static int get_device_type(SCSIDiskState *s)
>  static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>  {
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
> +    AioContext *ctx;
>      int sg_version;
>      int rc;
>  
> @@ -2568,7 +2579,10 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>      }
>  
>      /* check we are using a driver managing SG_IO (version 3 and after) */
> +    ctx = blk_get_aio_context(s->qdev.conf.blk);
> +    aio_context_acquire(ctx);
>      rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
> +    aio_context_release(ctx);
>      if (rc < 0) {
>          error_setg_errno(errp, -rc, "cannot get SG_IO version number");
>          if (rc != -EPERM) {

This is probably not enough. get_device_type() and
scsi_generic_read_device_inquiry() below issue more ioctls (but we need
to be careful not to include the scsi_realize() call in the locked
section if you take the lock again there).

Kevin

Alberto Garcia Jan. 11, 2019, 3:05 p.m. UTC | #2

On Fri 11 Jan 2019 04:02:13 PM CET, Kevin Wolf wrote:
>> @@ -2568,7 +2579,10 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>>      }
>>  
>>      /* check we are using a driver managing SG_IO (version 3 and after) */
>> +    ctx = blk_get_aio_context(s->qdev.conf.blk);
>> +    aio_context_acquire(ctx);
>>      rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
>> +    aio_context_release(ctx);
>>      if (rc < 0) {
>>          error_setg_errno(errp, -rc, "cannot get SG_IO version number");
>>          if (rc != -EPERM) {
>
> This is probably not enough. get_device_type() and
> scsi_generic_read_device_inquiry() below issue more ioctls (but we
> need to be careful not to include the scsi_realize() call in the
> locked section if you take the lock again there).

Hmmm, another alternative is not to take the lock in scsi_realize() and
take it instead in all the functions that call it (it's 4 or 5).

Berto

Kevin Wolf Jan. 11, 2019, 3:14 p.m. UTC | #3

Am 11.01.2019 um 16:05 hat Alberto Garcia geschrieben:
> On Fri 11 Jan 2019 04:02:13 PM CET, Kevin Wolf wrote:
> >> @@ -2568,7 +2579,10 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
> >>      }
> >>  
> >>      /* check we are using a driver managing SG_IO (version 3 and after) */
> >> +    ctx = blk_get_aio_context(s->qdev.conf.blk);
> >> +    aio_context_acquire(ctx);
> >>      rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
> >> +    aio_context_release(ctx);
> >>      if (rc < 0) {
> >>          error_setg_errno(errp, -rc, "cannot get SG_IO version number");
> >>          if (rc != -EPERM) {
> >
> > This is probably not enough. get_device_type() and
> > scsi_generic_read_device_inquiry() below issue more ioctls (but we
> > need to be careful not to include the scsi_realize() call in the
> > locked section if you take the lock again there).
> 
> Hmmm, another alternative is not to take the lock in scsi_realize() and
> take it instead in all the functions that call it (it's 4 or 5).

We touch most of them anyway, so I think it is an option.

Kevin

Patch

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 0e9027c8f3..8a22def7f3 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2318,16 +2318,20 @@  static void scsi_disk_unit_attention_reported(SCSIDevice *dev)
 static void scsi_realize(SCSIDevice *dev, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
+    AioContext *ctx;
 
     if (!s->qdev.conf.blk) {
         error_setg(errp, "drive property not set");
         return;
     }
 
+    ctx = blk_get_aio_context(s->qdev.conf.blk);
+    aio_context_acquire(ctx);
+
     if (!(s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
         !blk_is_inserted(s->qdev.conf.blk)) {
         error_setg(errp, "Device needs media, but drive is empty");
-        return;
+        goto out;
     }
 
     blkconf_blocksizes(&s->qdev.conf);
@@ -2336,18 +2340,18 @@  static void scsi_realize(SCSIDevice *dev, Error **errp)
         s->qdev.conf.physical_block_size) {
         error_setg(errp,
                    "logical_block_size > physical_block_size not supported");
-        return;
+        goto out;
     }
 
     if (dev->type == TYPE_DISK) {
         if (!blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, errp)) {
-            return;
+            goto out;
         }
     }
     if (!blkconf_apply_backend_options(&dev->conf,
                                        blk_is_read_only(s->qdev.conf.blk),
                                        dev->type == TYPE_DISK, errp)) {
-        return;
+        goto out;
     }
 
     if (s->qdev.conf.discard_granularity == -1) {
@@ -2364,7 +2368,7 @@  static void scsi_realize(SCSIDevice *dev, Error **errp)
 
     if (blk_is_sg(s->qdev.conf.blk)) {
         error_setg(errp, "unwanted /dev/sg*");
-        return;
+        goto out;
     }
 
     if ((s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
@@ -2376,6 +2380,9 @@  static void scsi_realize(SCSIDevice *dev, Error **errp)
     blk_set_guest_block_size(s->qdev.conf.blk, s->qdev.blocksize);
 
     blk_iostatus_enable(s->qdev.conf.blk);
+
+out:
+    aio_context_release(ctx);
 }
 
 static void scsi_hd_realize(SCSIDevice *dev, Error **errp)
@@ -2385,7 +2392,10 @@  static void scsi_hd_realize(SCSIDevice *dev, Error **errp)
      * backend will be issued in scsi_realize
      */
     if (s->qdev.conf.blk) {
+        AioContext *ctx = blk_get_aio_context(s->qdev.conf.blk);
+        aio_context_acquire(ctx);
         blkconf_blocksizes(&s->qdev.conf);
+        aio_context_release(ctx);
     }
     s->qdev.blocksize = s->qdev.conf.logical_block_size;
     s->qdev.type = TYPE_DISK;
@@ -2553,6 +2563,7 @@  static int get_device_type(SCSIDiskState *s)
 static void scsi_block_realize(SCSIDevice *dev, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
+    AioContext *ctx;
     int sg_version;
     int rc;
 
@@ -2568,7 +2579,10 @@  static void scsi_block_realize(SCSIDevice *dev, Error **errp)
     }
 
     /* check we are using a driver managing SG_IO (version 3 and after) */
+    ctx = blk_get_aio_context(s->qdev.conf.blk);
+    aio_context_acquire(ctx);
     rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
+    aio_context_release(ctx);
     if (rc < 0) {
         error_setg_errno(errp, -rc, "cannot get SG_IO version number");
         if (rc != -EPERM) {