diff mbox

[PULL,54/61] blockdev: Remove IF_* check for read-only blockdev_init

Message ID 1381503951-27985-55-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf Oct. 11, 2013, 3:05 p.m. UTC
IF_NONE allows read-only, which makes forbidding it in this place
for other types pretty much pointless.

Instead, make sure that all devices for which the check would have
errored out check in their init function that they don't get a read-only
BlockDriverState. This catches even cases where IF_NONE and -device is
used.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c                 | 6 ------
 hw/block/m25p80.c          | 5 +++++
 hw/block/xen_disk.c        | 5 +++++
 hw/sd/milkymist-memcard.c  | 4 ++++
 hw/sd/omap_mmc.c           | 6 ++++++
 hw/sd/pl181.c              | 4 ++++
 hw/sd/pxa2xx_mmci.c        | 3 +++
 hw/sd/sd.c                 | 5 +++++
 hw/sd/sdhci.c              | 3 +++
 hw/sd/ssi-sd.c             | 3 +++
 tests/qemu-iotests/051.out | 5 ++++-
 11 files changed, 42 insertions(+), 7 deletions(-)

Comments

Stefan Weil Oct. 15, 2013, 3:53 p.m. UTC | #1
Am 11.10.2013 17:05, schrieb Kevin Wolf:
> IF_NONE allows read-only, which makes forbidding it in this place
> for other types pretty much pointless.
>
> Instead, make sure that all devices for which the check would have
> errored out check in their init function that they don't get a read-only
> BlockDriverState. This catches even cases where IF_NONE and -device is
> used.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>

This patch breaks current QEMU (SIGSEGV with ARM in several test scenarios):

$ git bisect bad
4f8a066b5fc254eeaabbbde56ba4f5b29cc68fdf is the first bad commit
commit 4f8a066b5fc254eeaabbbde56ba4f5b29cc68fdf
Author: Kevin Wolf <kwolf@redhat.com>
Date:   Fri Sep 13 15:51:47 2013 +0200

    blockdev: Remove IF_* check for read-only blockdev_init
[...]

See the gdb protocol below for more details (Linux x86_64 host, default
configuration).

I got a bug report from a Windows user, but the crash is not OS specific.

Regards,
Stefan


(gdb) r
Starting program: bin/arm-softmmu/qemu-system-arm -M versatilepb -L
pc-bios -kernel vmlinuz-2.6.32-5-versatile -initrd
initrd.img-2.6.32-5-versatile -sd debian_squeeze_armel_standard.qcow2
-append root=/dev/sda1
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Breakpoint 5, pl181_init (sbd=0x5555565d3020) at hw/sd/pl181.c:482
482     {
(gdb) i s
#0  pl181_init (sbd=0x5555565d3020) at hw/sd/pl181.c:482
#1  0x00005555556d10e8 in sysbus_device_init (dev=0x5555565d3020) at
hw/core/sysbus.c:143
#2  0x00005555556ce6d3 in device_realize (dev=0x5555565d3020,
err=0x7fffffffdb08) at hw/core/qdev.c:178
#3  0x00005555556d002a in device_set_realized (obj=0x5555565d3020,
value=true, err=0x7fffffffdc80) at hw/core/qdev.c:699
#4  0x0000555555849520 in property_set_bool (obj=0x5555565d3020,
v=0x5555565d54d0, opaque=0x5555565ca870, name=0x555555a3c186 "realized",
errp=0x7fffffffdc80) at qom/object.c:1315
#5  0x0000555555848065 in object_property_set (obj=0x5555565d3020,
v=0x5555565d54d0, name=0x555555a3c186 "realized", errp=0x7fffffffdc80)
at qom/object.c:803
#6  0x00005555558497ca in object_property_set_qobject
(obj=0x5555565d3020, value=0x5555565adc20, name=0x555555a3c186
"realized", errp=0x7fffffffdc80) at qom/qom-qobject.c:24
#7  0x0000555555848351 in object_property_set_bool (obj=0x5555565d3020,
value=true, name=0x555555a3c186 "realized", errp=0x7fffffffdc80) at
qom/object.c:866
#8  0x00005555556ce60f in qdev_init (dev=0x5555565d3020) at
hw/core/qdev.c:163
#9  0x00005555556ceb8e in qdev_init_nofail (dev=0x5555565d3020) at
hw/core/qdev.c:277
#10 0x00005555556d11c3 in sysbus_create_varargs (name=0x555555a87f74
"pl181", addr=268455936) at hw/core/sysbus.c:157
#11 0x0000555555901572 in versatile_init (args=0x7fffffffe2d0,
board_id=387) at hw/arm/versatilepb.c:284
#12 0x0000555555901835 in vpb_init (args=0x7fffffffe2d0) at
hw/arm/versatilepb.c:357
#13 0x000055555589a38b in main (argc=13, argv=0x7fffffffe508,
envp=0x7fffffffe578) at vl.c:4245
(gdb) c
Continuing.

Breakpoint 5, pl181_init (sbd=0x5555565deda0) at hw/sd/pl181.c:482
482     {
(gdb)
Continuing.

Program received signal SIGSEGV, Segmentation fault.
0x000055555560b25f in bdrv_is_read_only (bs=0x0) at block.c:2933
2933        return bs->read_only;
(gdb) i s
#0  0x000055555560b25f in bdrv_is_read_only (bs=0x0) at block.c:2933
#1  0x0000555555794220 in sd_init (bs=0x0, is_spi=false) at hw/sd/sd.c:497
#2  0x000055555579316e in pl181_init (sbd=0x5555565deda0) at
hw/sd/pl181.c:493
#3  0x00005555556d10e8 in sysbus_device_init (dev=0x5555565deda0) at
hw/core/sysbus.c:143
#4  0x00005555556ce6d3 in device_realize (dev=0x5555565deda0,
err=0x7fffffffdb08) at hw/core/qdev.c:178
#5  0x00005555556d002a in device_set_realized (obj=0x5555565deda0,
value=true, err=0x7fffffffdc80) at hw/core/qdev.c:699
#6  0x0000555555849520 in property_set_bool (obj=0x5555565deda0,
v=0x5555565e1250, opaque=0x5555565ca500, name=0x555555a3c186 "realized",
errp=0x7fffffffdc80) at qom/object.c:1315
#7  0x0000555555848065 in object_property_set (obj=0x5555565deda0,
v=0x5555565e1250, name=0x555555a3c186 "realized", errp=0x7fffffffdc80)
at qom/object.c:803
#8  0x00005555558497ca in object_property_set_qobject
(obj=0x5555565deda0, value=0x5555565ca5f0, name=0x555555a3c186
"realized", errp=0x7fffffffdc80) at qom/qom-qobject.c:24
#9  0x0000555555848351 in object_property_set_bool (obj=0x5555565deda0,
value=true, name=0x555555a3c186 "realized", errp=0x7fffffffdc80) at
qom/object.c:866
#10 0x00005555556ce60f in qdev_init (dev=0x5555565deda0) at
hw/core/qdev.c:163
#11 0x00005555556ceb8e in qdev_init_nofail (dev=0x5555565deda0) at
hw/core/qdev.c:277
#12 0x00005555556d11c3 in sysbus_create_varargs (name=0x555555a87f74
"pl181", addr=268480512) at hw/core/sysbus.c:157
#13 0x000055555590159f in versatile_init (args=0x7fffffffe2d0,
board_id=387) at hw/arm/versatilepb.c:285
#14 0x0000555555901835 in vpb_init (args=0x7fffffffe2d0) at
hw/arm/versatilepb.c:357
#15 0x000055555589a38b in main (argc=13, argv=0x7fffffffe508,
envp=0x7fffffffe578) at vl.c:4245
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index b11155c..401ee25 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -528,12 +528,6 @@  static DriveInfo *blockdev_init(QDict *bs_opts,
     if (media == MEDIA_CDROM) {
         /* CDROM is fine for any interface, don't check.  */
         ro = 1;
-    } else if (ro == 1) {
-        if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
-            type != IF_NONE && type != IF_PFLASH) {
-            error_report("read-only not supported by this bus type");
-            goto err;
-        }
     }
 
     bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 8c3b7f0..02a1544 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -624,6 +624,11 @@  static int m25p80_init(SSISlave *ss)
     if (dinfo && dinfo->bdrv) {
         DB_PRINT_L(0, "Binding to IF_MTD drive\n");
         s->bdrv = dinfo->bdrv;
+        if (bdrv_is_read_only(s->bdrv)) {
+            fprintf(stderr, "Can't use a read-only drive");
+            return 1;
+        }
+
         /* FIXME: Move to late init */
         if (bdrv_read(s->bdrv, 0, s->storage, DIV_ROUND_UP(s->size,
                                                     BDRV_SECTOR_SIZE))) {
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 8742294..098f6c6 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -830,6 +830,11 @@  static int blk_connect(struct XenDevice *xendev)
         /* setup via qemu cmdline -> already setup for us */
         xen_be_printf(&blkdev->xendev, 2, "get configured bdrv (cmdline setup)\n");
         blkdev->bs = blkdev->dinfo->bdrv;
+        if (bdrv_is_read_only(blkdev->bs) && !readonly) {
+            xen_be_printf(&blkdev->xendev, 0, "Unexpected read-only drive");
+            blkdev->bs = NULL;
+            return -1;
+        }
         /* blkdev->bs is not create by us, we get a reference
          * so we can bdrv_unref() unconditionally */
         bdrv_ref(blkdev->bs);
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 42613b3..d1168c9 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -255,6 +255,10 @@  static int milkymist_memcard_init(SysBusDevice *dev)
 
     dinfo = drive_get_next(IF_SD);
     s->card = sd_init(dinfo ? dinfo->bdrv : NULL, false);
+    if (s->card == NULL) {
+        return -1;
+    }
+
     s->enabled = dinfo ? bdrv_is_inserted(dinfo->bdrv) : 0;
 
     memory_region_init_io(&s->regs_region, OBJECT(s), &memcard_mmio_ops, s,
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index bf5d1fb..937a478 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -593,6 +593,9 @@  struct omap_mmc_s *omap_mmc_init(hwaddr base,
 
     /* Instantiate the storage */
     s->card = sd_init(bd, false);
+    if (s->card == NULL) {
+        exit(1);
+    }
 
     return s;
 }
@@ -618,6 +621,9 @@  struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta,
 
     /* Instantiate the storage */
     s->card = sd_init(bd, false);
+    if (s->card == NULL) {
+        exit(1);
+    }
 
     s->cdet = qemu_allocate_irqs(omap_mmc_cover_cb, s, 1)[0];
     sd_set_cb(s->card, NULL, s->cdet);
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 03875bf..c35896d 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -491,6 +491,10 @@  static int pl181_init(SysBusDevice *sbd)
     qdev_init_gpio_out(dev, s->cardstatus, 2);
     dinfo = drive_get_next(IF_SD);
     s->card = sd_init(dinfo ? dinfo->bdrv : NULL, false);
+    if (s->card == NULL) {
+        return -1;
+    }
+
     return 0;
 }
 
diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 90c955f..b9d8b1a 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -539,6 +539,9 @@  PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
 
     /* Instantiate the actual storage */
     s->card = sd_init(bd, false);
+    if (s->card == NULL) {
+        exit(1);
+    }
 
     register_savevm(NULL, "pxa2xx_mmci", 0, 0,
                     pxa2xx_mmci_save, pxa2xx_mmci_load, s);
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 346d86f..7380f06 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -494,6 +494,11 @@  SDState *sd_init(BlockDriverState *bs, bool is_spi)
 {
     SDState *sd;
 
+    if (bdrv_is_read_only(bs)) {
+        fprintf(stderr, "sd_init: Cannot use read-only drive\n");
+        return NULL;
+    }
+
     sd = (SDState *) g_malloc0(sizeof(SDState));
     sd->buf = qemu_blockalign(bs, 512);
     sd->spi = is_spi;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 1483e19..0906a1d 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1166,6 +1166,9 @@  static void sdhci_initfn(Object *obj)
 
     di = drive_get_next(IF_SD);
     s->card = sd_init(di ? di->bdrv : NULL, false);
+    if (s->card == NULL) {
+        exit(1);
+    }
     s->eject_cb = qemu_allocate_irqs(sdhci_insert_eject_cb, s, 1)[0];
     s->ro_cb = qemu_allocate_irqs(sdhci_card_readonly_cb, s, 1)[0];
     sd_set_cb(s->card, s->ro_cb, s->eject_cb);
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index d47e237..1bb56c4 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -246,6 +246,9 @@  static int ssi_sd_init(SSISlave *dev)
     s->mode = SSI_SD_CMD;
     dinfo = drive_get_next(IF_SD);
     s->sd = sd_init(dinfo ? dinfo->bdrv : NULL, true);
+    if (s->sd == NULL) {
+        return -1;
+    }
     register_savevm(&dev->qdev, "ssi_sd", -1, 1, ssi_sd_save, ssi_sd_load, s);
     return 0;
 }
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index e58776a..2839e32 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -139,7 +139,10 @@  QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) qququiquit
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=ide,readonly=on
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=ide,readonly=on: read-only not supported by this bus type
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) QEMU_PROG: Can't use a read-only drive
+QEMU_PROG: Device initialization failed.
+QEMU_PROG: Initialization of device ide-hd failed
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=virtio,readonly=on
 QEMU X.Y.Z monitor - type 'help' for more information