[Qemu-devel] [PULL 00/14] SCSI changes for 1.1

[Qemu-devel] [PULL 00/14] SCSI changes for 1.1

[Paolo Bonzini]

Anthony,

the following changes since commit f05ae5379e40f81a6c8526d891693af8bf6e62da:

  Bail out if CONFIG_TCG_PASS_AREG0 is defined (2012-05-03 15:48:49 +0400)

are available in the git repository at:

  git://github.com/bonzini/qemu.git scsi-next

for you to fetch changes up to 537b10a444015fb6b01150f2ec7425a61472c621:

  scsi: Add assertion for use-after-free errors (2012-05-04 10:29:31 +0200)

With the patches, scsi-testsuite passes.

----------------------------------------------------------------
Paolo Bonzini (11):
      scsi: prevent data transfer overflow
      scsi: fix refcounting for reads
      scsi: fix WRITE SAME transfer length and direction
      scsi: change "removable" field to host many features
      scsi-disk: add dpofua property
      scsi: do not report bogus overruns for commands in the 0x00-0x1F range
      scsi: parse 16-byte tape CDBs
      scsi: do not require a minimum allocation length for INQUIRY
      scsi: do not require a minimum allocation length for REQUEST SENSE
      scsi: set VALID bit to 0 in fixed format sense data
      scsi: remove useless debug messages

Ronnie Sahlberg (2):
      ISCSI: Add support for thin-provisioning via discard/UNMAP and bigger LUNs
      scsi: Specify the xfer direction for UNMAP and ATA_PASSTHROUGH commands

Stefan Weil (1):
      scsi: Add assertion for use-after-free errors

 block/iscsi.c  |   86 ++++++++++++++++++++++++++++++++++++++++--------
 configure      |    5 ++-
 hw/scsi-bus.c  |  100 ++++++++++++++++++++++++++++++++++----------------------
 hw/scsi-defs.h |    1 +
 hw/scsi-disk.c |   66 ++++++++++++++++++-------------------
 5 files changed, 171 insertions(+), 87 deletions(-)
-- 
1.7.9.3

[Qemu-devel] [PATCH 01/14] ISCSI: Add support for thin-provisioning via discard/UNMAP and bigger LUNs

[Paolo Bonzini]

From: Ronnie Sahlberg <ronniesahlberg@gmail.com>

Update the configure test for libiscsi support to detect version 1.3
or later.  Version 1.3 of libiscsi provides both READCAPACITY16 as well
as UNMAP commands.

Update the iscsi block layer to use READCAPACITY16 to detect the size of
the LUN instead of READCAPACITY10. This allows support for LUNs larger
than 2TB.

Update to implement bdrv_aio_discard() using the UNMAP command.
This allows us to use thin-provisioned LUNs from TGTD and other iSCSI
targets that support thin-provisioning.

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
[squashed in and fixed subsequent patch from Ronnie to fix off-by-one in
 LBA count]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c |   86 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 configure     |    5 +++-
 2 files changed, 77 insertions(+), 14 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 5222726..d37c4ee 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -383,6 +383,65 @@ iscsi_aio_flush(BlockDriverState *bs,
     return &acb->common;
 }
 
+static void
+iscsi_unmap_cb(struct iscsi_context *iscsi, int status,
+                     void *command_data, void *opaque)
+{
+    IscsiAIOCB *acb = opaque;
+
+    if (acb->canceled != 0) {
+        qemu_aio_release(acb);
+        scsi_free_scsi_task(acb->task);
+        acb->task = NULL;
+        return;
+    }
+
+    acb->status = 0;
+    if (status < 0) {
+        error_report("Failed to unmap data on iSCSI lun. %s",
+                     iscsi_get_error(iscsi));
+        acb->status = -EIO;
+    }
+
+    iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
+    scsi_free_scsi_task(acb->task);
+    acb->task = NULL;
+}
+
+static BlockDriverAIOCB *
+iscsi_aio_discard(BlockDriverState *bs,
+                  int64_t sector_num, int nb_sectors,
+                  BlockDriverCompletionFunc *cb, void *opaque)
+{
+    IscsiLun *iscsilun = bs->opaque;
+    struct iscsi_context *iscsi = iscsilun->iscsi;
+    IscsiAIOCB *acb;
+    struct unmap_list list[1];
+
+    acb = qemu_aio_get(&iscsi_aio_pool, bs, cb, opaque);
+
+    acb->iscsilun = iscsilun;
+    acb->canceled   = 0;
+
+    list[0].lba = sector_qemu2lun(sector_num, iscsilun);
+    list[0].num = nb_sectors * BDRV_SECTOR_SIZE / iscsilun->block_size;
+
+    acb->task = iscsi_unmap_task(iscsi, iscsilun->lun,
+                                 0, 0, &list[0], 1,
+                                 iscsi_unmap_cb,
+                                 acb);
+    if (acb->task == NULL) {
+        error_report("iSCSI: Failed to send unmap command. %s",
+                     iscsi_get_error(iscsi));
+        qemu_aio_release(acb);
+        return NULL;
+    }
+
+    iscsi_set_events(iscsilun);
+
+    return &acb->common;
+}
+
 static int64_t
 iscsi_getlength(BlockDriverState *bs)
 {
@@ -396,11 +455,11 @@ iscsi_getlength(BlockDriverState *bs)
 }
 
 static void
-iscsi_readcapacity10_cb(struct iscsi_context *iscsi, int status,
+iscsi_readcapacity16_cb(struct iscsi_context *iscsi, int status,
                         void *command_data, void *opaque)
 {
     struct IscsiTask *itask = opaque;
-    struct scsi_readcapacity10 *rc10;
+    struct scsi_readcapacity16 *rc16;
     struct scsi_task *task = command_data;
 
     if (status != 0) {
@@ -412,26 +471,25 @@ iscsi_readcapacity10_cb(struct iscsi_context *iscsi, int status,
         return;
     }
 
-    rc10 = scsi_datain_unmarshall(task);
-    if (rc10 == NULL) {
-        error_report("iSCSI: Failed to unmarshall readcapacity10 data.");
+    rc16 = scsi_datain_unmarshall(task);
+    if (rc16 == NULL) {
+        error_report("iSCSI: Failed to unmarshall readcapacity16 data.");
         itask->status   = 1;
         itask->complete = 1;
         scsi_free_scsi_task(task);
         return;
     }
 
-    itask->iscsilun->block_size = rc10->block_size;
-    itask->iscsilun->num_blocks = rc10->lba;
-    itask->bs->total_sectors = (uint64_t)rc10->lba *
-                               rc10->block_size / BDRV_SECTOR_SIZE ;
+    itask->iscsilun->block_size = rc16->block_length;
+    itask->iscsilun->num_blocks = rc16->returned_lba + 1;
+    itask->bs->total_sectors    = itask->iscsilun->num_blocks *
+                               itask->iscsilun->block_size / BDRV_SECTOR_SIZE ;
 
     itask->status   = 0;
     itask->complete = 1;
     scsi_free_scsi_task(task);
 }
 
-
 static void
 iscsi_connect_cb(struct iscsi_context *iscsi, int status, void *command_data,
                  void *opaque)
@@ -445,10 +503,10 @@ iscsi_connect_cb(struct iscsi_context *iscsi, int status, void *command_data,
         return;
     }
 
-    task = iscsi_readcapacity10_task(iscsi, itask->iscsilun->lun, 0, 0,
-                                   iscsi_readcapacity10_cb, opaque);
+    task = iscsi_readcapacity16_task(iscsi, itask->iscsilun->lun,
+                                   iscsi_readcapacity16_cb, opaque);
     if (task == NULL) {
-        error_report("iSCSI: failed to send readcapacity command.");
+        error_report("iSCSI: failed to send readcapacity16 command.");
         itask->status   = 1;
         itask->complete = 1;
         return;
@@ -700,6 +758,8 @@ static BlockDriver bdrv_iscsi = {
     .bdrv_aio_readv  = iscsi_aio_readv,
     .bdrv_aio_writev = iscsi_aio_writev,
     .bdrv_aio_flush  = iscsi_aio_flush,
+
+    .bdrv_aio_discard = iscsi_aio_discard,
 };
 
 static void iscsi_block_init(void)
diff --git a/configure b/configure
index 0111774..0e3615b 100755
--- a/configure
+++ b/configure
@@ -2546,10 +2546,13 @@ fi
 
 ##########################################
 # Do we have libiscsi
+# We check for iscsi_unmap_sync() to make sure we have a
+# recent enough version of libiscsi.
 if test "$libiscsi" != "no" ; then
   cat > $TMPC << EOF
+#include <stdio.h>
 #include <iscsi/iscsi.h>
-int main(void) { iscsi_create_context(""); return 0; }
+int main(void) { iscsi_unmap_sync(NULL,0,0,0,NULL,0); return 0; }
 EOF
   if compile_prog "-Werror" "-liscsi" ; then
     libiscsi="yes"
-- 
1.7.9.3

[Qemu-devel] [PATCH 02/14] scsi: prevent data transfer overflow

[Paolo Bonzini]

Avoid sending more than 2GB of data, as that can cause overflows
in int32_t variables.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c |   38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index dbdb99c..c29a4ae 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -239,6 +239,18 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus)
     return res;
 }
 
+static int32_t scsi_invalid_field(SCSIRequest *req, uint8_t *buf)
+{
+    scsi_req_build_sense(req, SENSE_CODE(INVALID_FIELD));
+    scsi_req_complete(req, CHECK_CONDITION);
+    return 0;
+}
+
+static const struct SCSIReqOps reqops_invalid_field = {
+    .size         = sizeof(SCSIRequest),
+    .send_command = scsi_invalid_field
+};
+
 /* SCSIReqOps implementation for invalid commands.  */
 
 static int32_t scsi_invalid_command(SCSIRequest *req, uint8_t *buf)
@@ -517,18 +529,20 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
                                       cmd.lba);
         }
 
-        if ((d->unit_attention.key == UNIT_ATTENTION ||
-             bus->unit_attention.key == UNIT_ATTENTION) &&
-            (buf[0] != INQUIRY &&
-             buf[0] != REPORT_LUNS &&
-             buf[0] != GET_CONFIGURATION &&
-             buf[0] != GET_EVENT_STATUS_NOTIFICATION &&
-
-             /*
-              * If we already have a pending unit attention condition,
-              * report this one before triggering another one.
-              */
-             !(buf[0] == REQUEST_SENSE && d->sense_is_ua))) {
+        if (cmd.xfer > INT32_MAX) {
+            req = scsi_req_alloc(&reqops_invalid_field, d, tag, lun, hba_private);
+        } else if ((d->unit_attention.key == UNIT_ATTENTION ||
+                   bus->unit_attention.key == UNIT_ATTENTION) &&
+                  (buf[0] != INQUIRY &&
+                   buf[0] != REPORT_LUNS &&
+                   buf[0] != GET_CONFIGURATION &&
+                   buf[0] != GET_EVENT_STATUS_NOTIFICATION &&
+
+                   /*
+                    * If we already have a pending unit attention condition,
+                    * report this one before triggering another one.
+                    */
+                   !(buf[0] == REQUEST_SENSE && d->sense_is_ua))) {
             req = scsi_req_alloc(&reqops_unit_attention, d, tag, lun,
                                  hba_private);
         } else if (lun != d->lun ||
-- 
1.7.9.3

Re: [Qemu-devel] [PATCH 02/14] scsi: prevent data transfer overflow

[Stefan Weil]

Am 04.05.2012 10:45, schrieb Paolo Bonzini:
> Avoid sending more than 2GB of data, as that can cause overflows
> in int32_t variables.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   hw/scsi-bus.c |   38 ++++++++++++++++++++++++++------------
>   1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
> index dbdb99c..c29a4ae 100644
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -239,6 +239,18 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus)
>       return res;
>   }
>
> +static int32_t scsi_invalid_field(SCSIRequest *req, uint8_t *buf)
> +{
> +    scsi_req_build_sense(req, SENSE_CODE(INVALID_FIELD));
> +    scsi_req_complete(req, CHECK_CONDITION);
> +    return 0;
> +}
> +
> +static const struct SCSIReqOps reqops_invalid_field = {
> +    .size         = sizeof(SCSIRequest),
> +    .send_command = scsi_invalid_field
> +};
> +
>   /* SCSIReqOps implementation for invalid commands.  */
>
>   static int32_t scsi_invalid_command(SCSIRequest *req, uint8_t *buf)
> @@ -517,18 +529,20 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
>                                         cmd.lba);
>           }
>
> -        if ((d->unit_attention.key == UNIT_ATTENTION ||
> -             bus->unit_attention.key == UNIT_ATTENTION)&&
> -            (buf[0] != INQUIRY&&
> -             buf[0] != REPORT_LUNS&&
> -             buf[0] != GET_CONFIGURATION&&
> -             buf[0] != GET_EVENT_STATUS_NOTIFICATION&&
> -
> -             /*
> -              * If we already have a pending unit attention condition,
> -              * report this one before triggering another one.
> -              */
> -             !(buf[0] == REQUEST_SENSE&&  d->sense_is_ua))) {
> +        if (cmd.xfer>  INT32_MAX) {
> +            req = scsi_req_alloc(&reqops_invalid_field, d, tag, lun, hba_private);

WARNING: line over 80 characters
#54: FILE: hw/scsi-bus.c:533:
+            req = scsi_req_alloc(&reqops_invalid_field, d, tag, lun, 
hba_private);

total: 0 errors, 1 warnings, 50 lines checked

0002-scsi-prevent-data-transfer-overflow.patch has style problems, 
please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.


> +        } else if ((d->unit_attention.key == UNIT_ATTENTION ||
> +                   bus->unit_attention.key == UNIT_ATTENTION)&&
> +                  (buf[0] != INQUIRY&&
> +                   buf[0] != REPORT_LUNS&&
> +                   buf[0] != GET_CONFIGURATION&&
> +                   buf[0] != GET_EVENT_STATUS_NOTIFICATION&&
> +
> +                   /*
> +                    * If we already have a pending unit attention condition,
> +                    * report this one before triggering another one.
> +                    */
> +                   !(buf[0] == REQUEST_SENSE&&  d->sense_is_ua))) {
>               req = scsi_req_alloc(&reqops_unit_attention, d, tag, lun,
>                                    hba_private);
>           } else if (lun != d->lun ||

Re: [Qemu-devel] [PATCH 02/14] scsi: prevent data transfer overflow

[Paolo Bonzini]

Il 04/05/2012 18:28, Stefan Weil ha scritto:
>>
>> -             !(buf[0] == REQUEST_SENSE&&  d->sense_is_ua))) {
>> +        if (cmd.xfer>  INT32_MAX) {
>> +            req = scsi_req_alloc(&reqops_invalid_field, d, tag, lun,
>> hba_private);
> 
> WARNING: line over 80 characters
> #54: FILE: hw/scsi-bus.c:533:
> +            req = scsi_req_alloc(&reqops_invalid_field, d, tag, lun,
> hba_private);
> 
> total: 0 errors, 1 warnings, 50 lines checked
> 
> 0002-scsi-prevent-data-transfer-overflow.patch has style problems,
> please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.

It's a warning for a reason...

Paolo

Re: [Qemu-devel] [PATCH 02/14] scsi: prevent data transfer overflow

[Stefan Weil]

Am 04.05.2012 18:29, schrieb Paolo Bonzini:
> Il 04/05/2012 18:28, Stefan Weil ha scritto:
>>> - !(buf[0] == REQUEST_SENSE&& d->sense_is_ua))) { + if (cmd.xfer> 
>>> INT32_MAX) { + req = scsi_req_alloc(&reqops_invalid_field, d, tag, 
>>> lun, hba_private); 
>> WARNING: line over 80 characters #54: FILE: hw/scsi-bus.c:533: + req 
>> = scsi_req_alloc(&reqops_invalid_field, d, tag, lun, hba_private); 
>> total: 0 errors, 1 warnings, 50 lines checked 
>> 0002-scsi-prevent-data-transfer-overflow.patch has style problems, 
>> please review. If any of these errors are false positives report them 
>> to the maintainer, see CHECKPATCH in MAINTAINERS. 
> It's a warning for a reason... Paolo

That's ok. It was just funny that a patch which prevents
data transfer overflow introduces a line overflow :-)

The tabs in patches 6 and 7 are more problematic.
Although they are not at the start of the line, tabs
should be completely avoided (that's my personal
understanding of QEMU's coding rules).

Existing code which violates that rule is no reason to
add more code with tabs. Exceptions should be possible
for code imported from other projects, maybe also
when existing code with tabs is extended.

But what is even more important than those tabs is
getting the fixes in qemu 1.1. I pulled them just
to see whether my scsi problem was fixed, and it's
git and checkpatch.pl which complains, not me :-)

Cheers,
Stefan

Re: [Qemu-devel] [PATCH 02/14] scsi: prevent data transfer overflow

[Paolo Bonzini]

Il 04/05/2012 18:51, Stefan Weil ha scritto:
> The tabs in patches 6 and 7 are more problematic.
> Although they are not at the start of the line, tabs
> should be completely avoided (that's my personal
> understanding of QEMU's coding rules).

ok, I pushed an updated patchset to scsi-next (commit
68bd348ade453821fd5378479e6718e69bf181f1).

Paolo

[Qemu-devel] [PATCH 03/14] scsi: fix refcounting for reads

[Paolo Bonzini]

Recently introduced FUA support also gave us a use-after-free
of the BlockAcctCookie within a SCSIDiskReq, due to unbalanced
reference counting.

The patch fixes this by making scsi_do_read look like a combination
of scsi_*_complete + scsi_*_data.  It does both a ref (like
scsi_read_data) and an unref (like scsi_flush_complete).

Reported-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index a029ab6..eca00a6 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -296,6 +296,13 @@ static void scsi_do_read(void *opaque, int ret)
         }
     }
 
+    if (r->req.io_canceled) {
+        return;
+    }
+
+    /* The request is used as the AIO opaque value, so add a ref.  */
+    scsi_req_ref(&r->req);
+
     if (r->req.sg) {
         dma_acct_start(s->qdev.conf.bs, &r->acct, r->req.sg, BDRV_ACCT_READ);
         r->req.resid -= r->req.sg->size;
-- 
1.7.9.3

[Qemu-devel] [PATCH 04/14] scsi: fix WRITE SAME transfer length and direction

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c  |   14 ++++++++------
 hw/scsi-disk.c |    5 ++++-
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index c29a4ae..5640aae 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -791,7 +791,8 @@ static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
     case MODE_SENSE:
         break;
     case WRITE_SAME_10:
-        cmd->xfer = 1;
+    case WRITE_SAME_16:
+        cmd->xfer = dev->blocksize;
         break;
     case READ_CAPACITY_10:
         cmd->xfer = 8;
@@ -909,6 +910,10 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu
 
 static void scsi_cmd_xfer_mode(SCSICommand *cmd)
 {
+    if (!cmd->xfer) {
+        cmd->mode = SCSI_XFER_NONE;
+        return;
+    }
     switch (cmd->buf[0]) {
     case WRITE_6:
     case WRITE_10:
@@ -934,6 +939,7 @@ static void scsi_cmd_xfer_mode(SCSICommand *cmd)
     case UPDATE_BLOCK:
     case WRITE_LONG_10:
     case WRITE_SAME_10:
+    case WRITE_SAME_16:
     case SEARCH_HIGH_12:
     case SEARCH_EQUAL_12:
     case SEARCH_LOW_12:
@@ -946,11 +952,7 @@ static void scsi_cmd_xfer_mode(SCSICommand *cmd)
         cmd->mode = SCSI_XFER_TO_DEV;
         break;
     default:
-        if (cmd->xfer)
-            cmd->mode = SCSI_XFER_FROM_DEV;
-        else {
-            cmd->mode = SCSI_XFER_NONE;
-        }
+        cmd->mode = SCSI_XFER_FROM_DEV;
         break;
     }
 }
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index eca00a6..fbb1041 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1566,8 +1566,11 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
         }
         break;
     case WRITE_SAME_10:
+        len = lduw_be_p(&buf[7]);
+        goto write_same;
     case WRITE_SAME_16:
-        len = r->req.cmd.xfer / s->qdev.blocksize;
+        len = ldl_be_p(&buf[10]) & 0xffffffffULL;
+    write_same:
 
         DPRINTF("WRITE SAME() (sector %" PRId64 ", count %d)\n",
                 r->req.cmd.lba, len);
-- 
1.7.9.3

[Qemu-devel] [PATCH 05/14] scsi: Specify the xfer direction for UNMAP and ATA_PASSTHROUGH commands

[Paolo Bonzini]

From: Ronnie Sahlberg <ronniesahlberg@gmail.com>

scsi_cmd_xfer_mode() is used to specify the xfer direction for SCSI
commands that come in from the guest.  If the direction is set incorrectly
this will eventually cause QEMU to kernel-panic the guest.

Add UNMAP and ATAPASSTHROUGH as commands that send data to the device.

Without this change, recent kernels will send both UNMAP as well
as ATAPASSTHROUGH commands to any /dev/sg* device, which due to the
incorrect xfer direction very quickly causes the guest kernel to crash.

Example causing a crash without the patch applied:

./x86_64-softmmu/qemu-system-x86_64 -m 1024 -enable-kvm -cdrom linuxmint-12-gnome-dvd-64bit.iso -drive file=/dev/sg4,if=scsi,bus=0,unit=6

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 5640aae..08d5088 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -940,6 +940,7 @@ static void scsi_cmd_xfer_mode(SCSICommand *cmd)
     case WRITE_LONG_10:
     case WRITE_SAME_10:
     case WRITE_SAME_16:
+    case UNMAP:
     case SEARCH_HIGH_12:
     case SEARCH_EQUAL_12:
     case SEARCH_LOW_12:
@@ -949,6 +950,7 @@ static void scsi_cmd_xfer_mode(SCSICommand *cmd)
     case SEND_DVD_STRUCTURE:
     case PERSISTENT_RESERVE_OUT:
     case MAINTENANCE_OUT:
+    case ATA_PASSTHROUGH:
         cmd->mode = SCSI_XFER_TO_DEV;
         break;
     default:
-- 
1.7.9.3

[Qemu-devel] [PATCH 06/14] scsi: change "removable" field to host many features

[Paolo Bonzini]

It is pointless to add a uint32_t field for every new feature.
Since we will need a new feature soon, convert accesses to "removable"
to look at bit 0 only.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |   23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index fbb1041..e04b469 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -61,10 +61,12 @@ typedef struct SCSIDiskReq {
     BlockAcctCookie acct;
 } SCSIDiskReq;
 
+#define SCSI_DISK_F_REMOVABLE	0
+
 struct SCSIDiskState
 {
     SCSIDevice qdev;
-    uint32_t removable;
+    uint32_t features;
     bool media_changed;
     bool media_event;
     bool eject_request;
@@ -669,7 +671,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
     memset(outbuf, 0, buflen);
 
     outbuf[0] = s->qdev.type & 0x1f;
-    outbuf[1] = s->removable ? 0x80 : 0;
+    outbuf[1] = (s->features & (1 << SCSI_DISK_F_REMOVABLE)) ? 0x80 : 0;
     if (s->qdev.type == TYPE_ROM) {
         memcpy(&outbuf[16], "QEMU CD-ROM     ", 16);
     } else {
@@ -1710,7 +1712,8 @@ static int scsi_initfn(SCSIDevice *dev)
         return -1;
     }
 
-    if (!s->removable && !bdrv_is_inserted(s->qdev.conf.bs)) {
+    if (!(s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
+        !bdrv_is_inserted(s->qdev.conf.bs)) {
         error_report("Device needs media, but drive is empty");
         return -1;
     }
@@ -1732,7 +1735,7 @@ static int scsi_initfn(SCSIDevice *dev)
         return -1;
     }
 
-    if (s->removable) {
+    if (s->features & (1 << SCSI_DISK_F_REMOVABLE)) {
         bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_cd_block_ops, s);
     }
     bdrv_set_buffer_alignment(s->qdev.conf.bs, s->qdev.blocksize);
@@ -1755,7 +1758,7 @@ static int scsi_cd_initfn(SCSIDevice *dev)
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
     s->qdev.blocksize = 2048;
     s->qdev.type = TYPE_ROM;
-    s->removable = true;
+    s->features |= 1 << SCSI_DISK_F_REMOVABLE;
     return scsi_initfn(&s->qdev);
 }
 
@@ -1828,7 +1831,9 @@ static int get_device_type(SCSIDiskState *s)
         return -1;
     }
     s->qdev.type = buf[0];
-    s->removable = (buf[1] & 0x80) != 0;
+    if (buf[1] & 0x80) {
+        s->features |= 1 << SCSI_DISK_F_REMOVABLE;
+    }
     return 0;
 }
 
@@ -1928,7 +1933,8 @@ static SCSIRequest *scsi_block_new_request(SCSIDevice *d, uint32_t tag,
 
 static Property scsi_hd_properties[] = {
     DEFINE_SCSI_DISK_PROPERTIES(),
-    DEFINE_PROP_BIT("removable", SCSIDiskState, removable, 0, false),
+    DEFINE_PROP_BIT("removable", SCSIDiskState, features,
+                    SCSI_DISK_F_REMOVABLE, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2030,7 +2036,8 @@ static TypeInfo scsi_block_info = {
 
 static Property scsi_disk_properties[] = {
     DEFINE_SCSI_DISK_PROPERTIES(),
-    DEFINE_PROP_BIT("removable", SCSIDiskState, removable, 0, false),
+    DEFINE_PROP_BIT("removable", SCSIDiskState, features,
+                    SCSI_DISK_F_REMOVABLE, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.7.9.3

Re: [Qemu-devel] [PATCH 06/14] scsi: change "removable" field to host many features

[Stefan Weil]

Am 04.05.2012 10:45, schrieb Paolo Bonzini:
> It is pointless to add a uint32_t field for every new feature.
> Since we will need a new feature soon, convert accesses to "removable"
> to look at bit 0 only.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   hw/scsi-disk.c |   23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index fbb1041..e04b469 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -61,10 +61,12 @@ typedef struct SCSIDiskReq {
>       BlockAcctCookie acct;
>   } SCSIDiskReq;
>
> +#define SCSI_DISK_F_REMOVABLE	0
> +

ERROR: code indent should never use tabs
#23: FILE: hw/scsi-disk.c:64:
+#define SCSI_DISK_F_REMOVABLE^I0$

total: 1 errors, 0 warnings, 74 lines checked

0006-scsi-change-removable-field-to-host-many-features.patch has style 
problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Re: [Qemu-devel] [PATCH 06/14] scsi: change "removable" field to host many features

[Paolo Bonzini]

Il 04/05/2012 18:30, Stefan Weil ha scritto:
>>
>> +#define SCSI_DISK_F_REMOVABLE    0
>> +
> 
> ERROR: code indent should never use tabs
> #23: FILE: hw/scsi-disk.c:64:
> +#define SCSI_DISK_F_REMOVABLE^I0$
> 
> total: 1 errors, 0 warnings, 74 lines checked

Not code indent.

$ git grep $'^#.*define.*\t' -- \*.c |wc -l
3244

Paolo

Re: [Qemu-devel] [PATCH 06/14] scsi: change "removable" field to host many features

[Andreas Färber]

Am 04.05.2012 18:36, schrieb Paolo Bonzini:
> Il 04/05/2012 18:30, Stefan Weil ha scritto:
>>>
>>> +#define SCSI_DISK_F_REMOVABLE    0
>>> +
>>
>> ERROR: code indent should never use tabs
>> #23: FILE: hw/scsi-disk.c:64:
>> +#define SCSI_DISK_F_REMOVABLE^I0$
>>
>> total: 1 errors, 0 warnings, 74 lines checked
> 
> Not code indent.
> 
> $ git grep $'^#.*define.*\t' -- \*.c |wc -l
> 3244

So? We add missing braces on the lines we touch as well. On the lines
I'm touching in my upcoming series I've thus space-converted such tabs.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 07/14] scsi-disk: add dpofua property

[Paolo Bonzini]

Linux expects REQ_FUA to be advertised only if WRITE+FUA is faster than
WRITE+SYNCHRONIZE CACHE, so we should not set the DPOFUA bit.  However,
it is useful to have it for testing purposes, so add a qdev property to
set it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index e04b469..e0f1821 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -62,6 +62,7 @@ typedef struct SCSIDiskReq {
 } SCSIDiskReq;
 
 #define SCSI_DISK_F_REMOVABLE	0
+#define SCSI_DISK_F_DPOFUA	1
 
 struct SCSIDiskState
 {
@@ -1103,7 +1104,7 @@ static int scsi_disk_emulate_mode_sense(SCSIDiskReq *r, uint8_t *outbuf)
     p = outbuf;
 
     if (s->qdev.type == TYPE_DISK) {
-        dev_specific_param = 0x10; /* DPOFUA */
+        dev_specific_param = s->features & (1 << SCSI_DISK_F_DPOFUA) ? 0x10 : 0;
         if (bdrv_is_read_only(s->qdev.conf.bs)) {
             dev_specific_param |= 0x80; /* Readonly.  */
         }
@@ -1935,6 +1936,8 @@ static Property scsi_hd_properties[] = {
     DEFINE_SCSI_DISK_PROPERTIES(),
     DEFINE_PROP_BIT("removable", SCSIDiskState, features,
                     SCSI_DISK_F_REMOVABLE, false),
+    DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
+                    SCSI_DISK_F_DPOFUA, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2038,6 +2041,8 @@ static Property scsi_disk_properties[] = {
     DEFINE_SCSI_DISK_PROPERTIES(),
     DEFINE_PROP_BIT("removable", SCSIDiskState, features,
                     SCSI_DISK_F_REMOVABLE, false),
+    DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
+                    SCSI_DISK_F_DPOFUA, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.7.9.3

Re: [Qemu-devel] [PATCH 07/14] scsi-disk: add dpofua property

[Stefan Weil]

Am 04.05.2012 10:45, schrieb Paolo Bonzini:
> Linux expects REQ_FUA to be advertised only if WRITE+FUA is faster than
> WRITE+SYNCHRONIZE CACHE, so we should not set the DPOFUA bit.  However,
> it is useful to have it for testing purposes, so add a qdev property to
> set it.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   hw/scsi-disk.c |    7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index e04b469..e0f1821 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -62,6 +62,7 @@ typedef struct SCSIDiskReq {
>   } SCSIDiskReq;
>
>   #define SCSI_DISK_F_REMOVABLE	0
> +#define SCSI_DISK_F_DPOFUA	1
>
>

ERROR: code indent should never use tabs
#24: FILE: hw/scsi-disk.c:65:
+#define SCSI_DISK_F_DPOFUA^I1$

total: 1 errors, 0 warnings, 31 lines checked

0007-scsi-disk-add-dpofua-property.patch has style problems, please 
review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

[Qemu-devel] [PATCH 08/14] scsi: do not report bogus overruns for commands in the 0x00-0x1F range

[Paolo Bonzini]

Interpreting cdb[4] == 0 as a request to transfer 256 blocks is only
needed for READ_6 and WRITE_6.  No other command in that range needs
that special-casing, and the resulting overrun breaks scsi-testsuite's
attempt to use command 2 as a known-invalid command.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 08d5088..5fbf8db 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -735,10 +735,6 @@ static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
     case 0:
         cmd->xfer = buf[4];
         cmd->len = 6;
-        /* length 0 means 256 blocks */
-        if (cmd->xfer == 0) {
-            cmd->xfer = 256;
-        }
         break;
     case 1:
     case 2:
@@ -808,18 +804,26 @@ static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
             cmd->xfer = buf[9] | (buf[8] << 8);
         }
         break;
+    case WRITE_6:
+        /* length 0 means 256 blocks */
+        if (cmd->xfer == 0) {
+            cmd->xfer = 256;
+        }
     case WRITE_10:
     case WRITE_VERIFY_10:
-    case WRITE_6:
     case WRITE_12:
     case WRITE_VERIFY_12:
     case WRITE_16:
     case WRITE_VERIFY_16:
         cmd->xfer *= dev->blocksize;
         break;
-    case READ_10:
     case READ_6:
     case READ_REVERSE:
+        /* length 0 means 256 blocks */
+        if (cmd->xfer == 0) {
+            cmd->xfer = 256;
+        }
+    case READ_10:
     case RECOVER_BUFFERED_DATA:
     case READ_12:
     case READ_16:
-- 
1.7.9.3

[Qemu-devel] [PATCH 09/14] scsi: parse 16-byte tape CDBs

[Paolo Bonzini]

The transfer length for these commands is different from the transfer
length of the corresponding disk commands, so parse it specially.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        READ REVERSE(16) seems to be for people who stream manga from tape.

 hw/scsi-bus.c  |   10 ++++++++++
 hw/scsi-defs.h |    1 +
 2 files changed, 11 insertions(+)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 5fbf8db..46cd1f9 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -891,6 +891,16 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu
             cmd->xfer *= dev->blocksize;
         }
         break;
+    case READ_16:
+    case READ_REVERSE_16:
+    case VERIFY_16:
+    case WRITE_16:
+        cmd->len = 16;
+        cmd->xfer = buf[14] | (buf[13] << 8) | (buf[12] << 16);
+        if (buf[1] & 0x01) { /* fixed */
+            cmd->xfer *= dev->blocksize;
+        }
+        break;
     case REWIND:
     case START_STOP:
         cmd->len = 6;
diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
index ca24192..219c84d 100644
--- a/hw/scsi-defs.h
+++ b/hw/scsi-defs.h
@@ -92,6 +92,7 @@
 #define PERSISTENT_RESERVE_OUT 0x5f
 #define VARLENGTH_CDB         0x7f
 #define WRITE_FILEMARKS_16    0x80
+#define READ_REVERSE_16       0x81
 #define ALLOW_OVERWRITE       0x82
 #define EXTENDED_COPY         0x83
 #define ATA_PASSTHROUGH       0x85
-- 
1.7.9.3

[Qemu-devel] [PATCH 10/14] scsi: do not require a minimum allocation length for INQUIRY

[Paolo Bonzini]

The requirements on the INQUIRY buffer size are not in my copy of SPC
(SPC-4 r27) and not observed by LIO.  Rip them out.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c  |    8 --------
 hw/scsi-disk.c |   11 -----------
 2 files changed, 19 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 46cd1f9..4090b9f 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -367,10 +367,6 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
     if (r->req.cmd.buf[1] & 0x1) {
         /* Vital product data */
         uint8_t page_code = r->req.cmd.buf[2];
-        if (r->req.cmd.xfer < 4) {
-            return false;
-        }
-
         r->buf[r->len++] = page_code ; /* this page */
         r->buf[r->len++] = 0x00;
 
@@ -398,10 +394,6 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
     }
 
     /* PAGE CODE == 0 */
-    if (r->req.cmd.xfer < 5) {
-        return false;
-    }
-
     r->len = MIN(r->req.cmd.xfer, 36);
     memset(r->buf, 0, r->len);
     if (r->req.lun != 0) {
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index e0f1821..298b4ef 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -524,11 +524,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
     if (req->cmd.buf[1] & 0x1) {
         /* Vital product data */
         uint8_t page_code = req->cmd.buf[2];
-        if (req->cmd.xfer < 4) {
-            BADF("Error: Inquiry (EVPD[%02X]) buffer size %zd is "
-                 "less than 4\n", page_code, req->cmd.xfer);
-            return -1;
-        }
 
         outbuf[buflen++] = s->qdev.type & 0x1f;
         outbuf[buflen++] = page_code ; // this page
@@ -659,12 +654,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
     }
 
     /* PAGE CODE == 0 */
-    if (req->cmd.xfer < 5) {
-        BADF("Error: Inquiry (STANDARD) buffer size %zd "
-             "is less than 5\n", req->cmd.xfer);
-        return -1;
-    }
-
     buflen = req->cmd.xfer;
     if (buflen > SCSI_MAX_INQUIRY_LEN) {
         buflen = SCSI_MAX_INQUIRY_LEN;
-- 
1.7.9.3

[Qemu-devel] [PATCH 11/14] scsi: do not require a minimum allocation length for REQUEST SENSE

[Paolo Bonzini]

The requirements on the REQUEST SENSE buffer size are not in my copy of SPC
(SPC-4 r27) and not observed by LIO.  Rip them out.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 4090b9f..925c3ae 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -427,9 +427,6 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
         }
         break;
     case REQUEST_SENSE:
-        if (req->cmd.xfer < 4) {
-            goto illegal_request;
-        }
         r->len = scsi_device_get_sense(r->req.dev, r->buf,
                                        MIN(req->cmd.xfer, sizeof r->buf),
                                        (req->cmd.buf[1] & 1) == 0);
@@ -538,8 +535,8 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
             req = scsi_req_alloc(&reqops_unit_attention, d, tag, lun,
                                  hba_private);
         } else if (lun != d->lun ||
-            buf[0] == REPORT_LUNS ||
-            (buf[0] == REQUEST_SENSE && (d->sense_len || cmd.xfer < 4))) {
+                   buf[0] == REPORT_LUNS ||
+                   (buf[0] == REQUEST_SENSE && d->sense_len)) {
             req = scsi_req_alloc(&reqops_target_command, d, tag, lun,
                                  hba_private);
         } else {
-- 
1.7.9.3

[Qemu-devel] [PATCH 12/14] scsi: set VALID bit to 0 in fixed format sense data

[Paolo Bonzini]

The INFORMATION field (bytes 3..6) is never set by QEMU, so the VALID
bit must be 0.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 925c3ae..add1d4f 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -649,7 +649,7 @@ void scsi_req_build_sense(SCSIRequest *req, SCSISense sense)
     trace_scsi_req_build_sense(req->dev->id, req->lun, req->tag,
                                sense.key, sense.asc, sense.ascq);
     memset(req->sense, 0, 18);
-    req->sense[0] = 0xf0;
+    req->sense[0] = 0x70;
     req->sense[2] = sense.key;
     req->sense[7] = 10;
     req->sense[12] = sense.asc;
@@ -1148,7 +1148,7 @@ int scsi_build_sense(uint8_t *in_buf, int in_len,
     memset(buf, 0, len);
     if (fixed) {
         /* Return fixed format sense buffer */
-        buf[0] = 0xf0;
+        buf[0] = 0x70;
         buf[2] = sense.key;
         buf[7] = 10;
         buf[12] = sense.asc;
-- 
1.7.9.3

[Qemu-devel] [PATCH 13/14] scsi: remove useless debug messages

[Paolo Bonzini]

Optional inquiry information is declared obsolete in the latest versions
of the standard; invalid CDBs or unsupported VPD pages are supported
can be diagnosed with trace_scsi_inquiry.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |   13 -------------
 1 file changed, 13 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 298b4ef..08a8226 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -28,9 +28,6 @@ do { printf("scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
 #define DPRINTF(fmt, ...) do {} while(0)
 #endif
 
-#define BADF(fmt, ...) \
-do { fprintf(stderr, "scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
-
 #include "qemu-common.h"
 #include "qemu-error.h"
 #include "scsi.h"
@@ -515,12 +512,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
     int buflen = 0;
 
-    if (req->cmd.buf[1] & 0x2) {
-        /* Command support data - optional, not implemented */
-        BADF("optional INQUIRY command support request not implemented\n");
-        return -1;
-    }
-
     if (req->cmd.buf[1] & 0x1) {
         /* Vital product data */
         uint8_t page_code = req->cmd.buf[2];
@@ -638,8 +629,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             break;
         }
         default:
-            BADF("Error: unsupported Inquiry (EVPD[%02X]) "
-                 "buffer size %zd\n", page_code, req->cmd.xfer);
             return -1;
         }
         /* done with EVPD */
@@ -648,8 +637,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
 
     /* Standard INQUIRY data */
     if (req->cmd.buf[2] != 0) {
-        BADF("Error: Inquiry (STANDARD) page or code "
-             "is non-zero [%02X]\n", req->cmd.buf[2]);
         return -1;
     }
 
-- 
1.7.9.3

[Qemu-devel] [PATCH 14/14] scsi: Add assertion for use-after-free errors

[Paolo Bonzini]

From: Stefan Weil <sw@weilnetz.de>

The QEMU emulation which is currently used with Raspberry PI images
(qemu-system-arm -M versatilepb ...) accesses memory which was freed.

Valgrind output (extract):

==17857== Invalid write of size 4
==17857==    at 0x24EB06: scsi_req_unref (scsi-bus.c:1273)
==17857==    by 0x24FFAE: scsi_read_complete (scsi-disk.c:277)
==17857==    by 0x152ACC: bdrv_co_em_bh (block.c:3363)
==17857==    by 0x13D49C: qemu_bh_poll (async.c:71)
==17857==    by 0x211A8C: main_loop_wait (main-loop.c:503)
==17857==    by 0x207954: main_loop (vl.c:1555)
==17857==    by 0x20E9C9: main (vl.c:3653)
==17857==  Address 0x1c54383c is 12 bytes inside a block of size 260 free'd
==17857==    at 0x4824B3A: free (vg_replace_malloc.c:366)
==17857==    by 0x20ADFA: free_and_trace (vl.c:2250)
==17857==    by 0x4899FC5: g_free (in /lib/libglib-2.0.so.0.2400.1)
==17857==    by 0x24EB3B: scsi_req_unref (scsi-bus.c:1277)
==17857==    by 0x24F003: scsi_req_complete (scsi-bus.c:1383)
==17857==    by 0x25022A: scsi_read_data (scsi-disk.c:334)
==17857==    by 0x24EB9F: scsi_req_continue (scsi-bus.c:1289)
==17857==    by 0x1C7787: lsi_do_dma (lsi53c895a.c:575)
==17857==    by 0x1C8CDA: lsi_execute_script (lsi53c895a.c:1147)
==17857==    by 0x1C74EA: lsi_resume_script (lsi53c895a.c:510)
==17857==    by 0x1C7ECD: lsi_transfer_data (lsi53c895a.c:746)
==17857==    by 0x24EC90: scsi_req_data (scsi-bus.c:1307)

(There are some more similar messages.)

This patch adds an assertion which also detects those errors:

Calling scsi_req_unref is not allowed when the previous call
of that function has decremented refcount to 0, because in this
case req was freed.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index add1d4f..8ab9bcd 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -1291,6 +1291,7 @@ SCSIRequest *scsi_req_ref(SCSIRequest *req)
 
 void scsi_req_unref(SCSIRequest *req)
 {
+    assert(req->refcount > 0);
     if (--req->refcount == 0) {
         if (req->ops->free_req) {
             req->ops->free_req(req);
-- 
1.7.9.3

Re: [Qemu-devel] [PULL 00/14] SCSI changes for 1.1

[Anthony Liguori]

On 05/04/2012 03:45 AM, Paolo Bonzini wrote:
> Anthony,
>
> the following changes since commit f05ae5379e40f81a6c8526d891693af8bf6e62da:
>
>    Bail out if CONFIG_TCG_PASS_AREG0 is defined (2012-05-03 15:48:49 +0400)
>
> are available in the git repository at:
>
>    git://github.com/bonzini/qemu.git scsi-next
>
> for you to fetch changes up to 537b10a444015fb6b01150f2ec7425a61472c621:
>
>    scsi: Add assertion for use-after-free errors (2012-05-04 10:29:31 +0200)
>
> With the patches, scsi-testsuite passes.

Pulled.  Thanks.

Regards,

Anthony Liguori

>
> ----------------------------------------------------------------
> Paolo Bonzini (11):
>        scsi: prevent data transfer overflow
>        scsi: fix refcounting for reads
>        scsi: fix WRITE SAME transfer length and direction
>        scsi: change "removable" field to host many features
>        scsi-disk: add dpofua property
>        scsi: do not report bogus overruns for commands in the 0x00-0x1F range
>        scsi: parse 16-byte tape CDBs
>        scsi: do not require a minimum allocation length for INQUIRY
>        scsi: do not require a minimum allocation length for REQUEST SENSE
>        scsi: set VALID bit to 0 in fixed format sense data
>        scsi: remove useless debug messages
>
> Ronnie Sahlberg (2):
>        ISCSI: Add support for thin-provisioning via discard/UNMAP and bigger LUNs
>        scsi: Specify the xfer direction for UNMAP and ATA_PASSTHROUGH commands
>
> Stefan Weil (1):
>        scsi: Add assertion for use-after-free errors
>
>   block/iscsi.c  |   86 ++++++++++++++++++++++++++++++++++++++++--------
>   configure      |    5 ++-
>   hw/scsi-bus.c  |  100 ++++++++++++++++++++++++++++++++++----------------------
>   hw/scsi-defs.h |    1 +
>   hw/scsi-disk.c |   66 ++++++++++++++++++-------------------
>   5 files changed, 171 insertions(+), 87 deletions(-)