diff mbox series

[v2,5/5] hw/scsi: Check sense key before READ CAPACITY output snoop

Message ID 20190717212703.10205-6-dmitry.fomichev@wdc.com (mailing list archive)
State New, archived
Headers show
Series virtio/block: handle zoned backing devices | expand

Commit Message

Dmitry Fomichev July 17, 2019, 9:27 p.m. UTC 
From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

When READ CAPACITY command completes, scsi_read_complete() function
snoops the command result and updates SCSIDevice members blocksize and
max_lba . However, this update is executed even when READ CAPACITY
command indicates an error in sense data. This causes unexpected
blocksize update with zero value for SCSI devices without
READ CAPACITY(10) command support and eventually results in a divide
by zero. An emulated device by TCMU-runner is an example of a device
that doesn't support READ CAPACITY(10) command.

To avoid the unexpected update, add sense key check in
scsi_read_complete() function. The function already checks the sense
key for VPD Block Limits emulation. Make scsi_parse_sense_buf() call
common for VPD Block Limits emulation and READ CAPACITY snoop. Update
blocksize and max_lba only if READ CAPACITY returns zero sense key.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 hw/scsi/scsi-generic.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

Comments

Paolo Bonzini July 18, 2019, 9:42 a.m. UTC | #1 
On 17/07/19 23:27, Dmitry Fomichev wrote:
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index a43efe39ec..e38d3160fa 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -238,6 +238,7 @@ static void scsi_read_complete(void * opaque, int ret)
>      SCSIGenericReq *r = (SCSIGenericReq *)opaque;
>      SCSIDevice *s = r->req.dev;
>      int len;
> +    uint8_t sense_key = NO_SENSE;
>  
>      assert(r->req.aiocb != NULL);
>      r->req.aiocb = NULL;
> @@ -254,6 +255,12 @@ static void scsi_read_complete(void * opaque, int ret)
>  
>      r->len = -1;
>  
> +    if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
> +        SCSISense sense =
> +            scsi_parse_sense_buf(r->req.sense, r->io_header.sb_len_wr);
> +        sense_key = sense.key;
> +    }
> +
>      /*
>       * Check if this is a VPD Block Limits request that
>       * resulted in sense error but would need emulation.
> @@ -264,9 +271,7 @@ static void scsi_read_complete(void * opaque, int ret)
>          r->req.cmd.buf[0] == INQUIRY &&
>          (r->req.cmd.buf[1] & 0x01) &&
>          r->req.cmd.buf[2] == 0xb0) {
> -        SCSISense sense =
> -            scsi_parse_sense_buf(r->req.sense, r->io_header.sb_len_wr);
> -        if (sense.key == ILLEGAL_REQUEST) {
> +        if (sense_key == ILLEGAL_REQUEST) {
>              len = scsi_generic_emulate_block_limits(r, s);
>              /*
>               * No need to let scsi_read_complete go on and handle an
> @@ -281,15 +286,17 @@ static void scsi_read_complete(void * opaque, int ret)
>          goto done;
>      }
>  
> -    /* Snoop READ CAPACITY output to set the blocksize.  */
> -    if (r->req.cmd.buf[0] == READ_CAPACITY_10 &&
> -        (ldl_be_p(&r->buf[0]) != 0xffffffffU || s->max_lba == 0)) {
> -        s->blocksize = ldl_be_p(&r->buf[4]);
> -        s->max_lba = ldl_be_p(&r->buf[0]) & 0xffffffffULL;
> -    } else if (r->req.cmd.buf[0] == SERVICE_ACTION_IN_16 &&
> -               (r->req.cmd.buf[1] & 31) == SAI_READ_CAPACITY_16) {
> -        s->blocksize = ldl_be_p(&r->buf[8]);
> -        s->max_lba = ldq_be_p(&r->buf[0]);
> +    /* Snoop READ CAPACITY output to set the blocksize. */
> +    if (sense_key == NO_SENSE) {

I think we can do better and skip all this snooping and patching if 
sense_key != 0.

In fact, the check for "r->io_header.driver_status & 
SG_ERR_DRIVER_SENSE" where we handle block limits is now duplicate with 
the one we do before setting sense_key.  With the extra cleanup that
ret == 0 has already been checked before, you get:

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index ccb632c476..7f066d4198 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -254,24 +254,28 @@ static void scsi_read_complete(void * opaque, int ret)
 
     r->len = -1;
 
-    /*
-     * Check if this is a VPD Block Limits request that
-     * resulted in sense error but would need emulation.
-     * In this case, emulate a valid VPD response.
-     */
-    if (s->needs_vpd_bl_emulation && ret == 0 &&
-        (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) &&
-        r->req.cmd.buf[0] == INQUIRY &&
-        (r->req.cmd.buf[1] & 0x01) &&
-        r->req.cmd.buf[2] == 0xb0) {
+    if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
         SCSISense sense =
             scsi_parse_sense_buf(r->req.sense, r->io_header.sb_len_wr);
-        if (sense.key == ILLEGAL_REQUEST) {
+
+        /*
+         * Check if this is a VPD Block Limits request that
+         * resulted in sense error but would need emulation.
+         * In this case, emulate a valid VPD response.
+         */
+        if (sense.key == ILLEGAL_REQUEST &&
+            s->needs_vpd_bl_emulation &&
+            r->req.cmd.buf[0] == INQUIRY &&
+            (r->req.cmd.buf[1] & 0x01) &&
+            r->req.cmd.buf[2] == 0xb0) {
             len = scsi_generic_emulate_block_limits(r, s);
             /*
-             * No need to let scsi_read_complete go on and handle an
+             * It's okay to jump to req_complete: no need to
+             * let scsi_handle_inquiry_reply handle an
              * INQUIRY VPD BL request we created manually.
              */
+        }
+        if (sense.key) {
             goto req_complete;
         }
     }

It is essentially swapping the two "if"s in the existing block limits
emulation code, which makes sense.  Looks good?

Paolo
diff mbox series

Patch

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index a43efe39ec..e38d3160fa 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -238,6 +238,7 @@  static void scsi_read_complete(void * opaque, int ret)
     SCSIGenericReq *r = (SCSIGenericReq *)opaque;
     SCSIDevice *s = r->req.dev;
     int len;
+    uint8_t sense_key = NO_SENSE;
 
     assert(r->req.aiocb != NULL);
     r->req.aiocb = NULL;
@@ -254,6 +255,12 @@  static void scsi_read_complete(void * opaque, int ret)
 
     r->len = -1;
 
+    if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
+        SCSISense sense =
+            scsi_parse_sense_buf(r->req.sense, r->io_header.sb_len_wr);
+        sense_key = sense.key;
+    }
+
     /*
      * Check if this is a VPD Block Limits request that
      * resulted in sense error but would need emulation.
@@ -264,9 +271,7 @@  static void scsi_read_complete(void * opaque, int ret)
         r->req.cmd.buf[0] == INQUIRY &&
         (r->req.cmd.buf[1] & 0x01) &&
         r->req.cmd.buf[2] == 0xb0) {
-        SCSISense sense =
-            scsi_parse_sense_buf(r->req.sense, r->io_header.sb_len_wr);
-        if (sense.key == ILLEGAL_REQUEST) {
+        if (sense_key == ILLEGAL_REQUEST) {
             len = scsi_generic_emulate_block_limits(r, s);
             /*
              * No need to let scsi_read_complete go on and handle an
@@ -281,15 +286,17 @@  static void scsi_read_complete(void * opaque, int ret)
         goto done;
     }
 
-    /* Snoop READ CAPACITY output to set the blocksize.  */
-    if (r->req.cmd.buf[0] == READ_CAPACITY_10 &&
-        (ldl_be_p(&r->buf[0]) != 0xffffffffU || s->max_lba == 0)) {
-        s->blocksize = ldl_be_p(&r->buf[4]);
-        s->max_lba = ldl_be_p(&r->buf[0]) & 0xffffffffULL;
-    } else if (r->req.cmd.buf[0] == SERVICE_ACTION_IN_16 &&
-               (r->req.cmd.buf[1] & 31) == SAI_READ_CAPACITY_16) {
-        s->blocksize = ldl_be_p(&r->buf[8]);
-        s->max_lba = ldq_be_p(&r->buf[0]);
+    /* Snoop READ CAPACITY output to set the blocksize. */
+    if (sense_key == NO_SENSE) {
+        if (r->req.cmd.buf[0] == READ_CAPACITY_10 &&
+            (ldl_be_p(&r->buf[0]) != 0xffffffffU || s->max_lba == 0)) {
+            s->blocksize = ldl_be_p(&r->buf[4]);
+            s->max_lba = ldl_be_p(&r->buf[0]) & 0xffffffffULL;
+        } else if (r->req.cmd.buf[0] == SERVICE_ACTION_IN_16 &&
+                   (r->req.cmd.buf[1] & 31) == SAI_READ_CAPACITY_16) {
+            s->blocksize = ldl_be_p(&r->buf[8]);
+            s->max_lba = ldq_be_p(&r->buf[0]);
+        }
     }
     blk_set_guest_block_size(s->conf.blk, s->blocksize);