diff mbox series

[10/10] scsi/scsi_bus: fix races in REPORT LUNS

Message ID 20200925172604.2142227-11-pbonzini@redhat.com (mailing list archive)
State New, archived
Headers show
Series Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread | expand

Commit Message

Paolo Bonzini Sept. 25, 2020, 5:26 p.m. UTC 
From: Maxim Levitsky <mlevitsk@redhat.com>

Currently scsi_target_emulate_report_luns iterates over the child device list
twice, and there is no guarantee that this list is the same in both iterations.

The reason for iterating twice is that the first iteration calculates
how much memory to allocate.  However if we use a dynamic array we can
avoid iterating twice, and therefore we avoid this race.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1866707

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-10-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-bus.c | 68 ++++++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 35 deletions(-)

Comments

Maxim Levitsky Sept. 30, 2020, 2:34 p.m. UTC | #1 
On Fri, 2020-09-25 at 13:26 -0400, Paolo Bonzini wrote:
> From: Maxim Levitsky <mlevitsk@redhat.com>
> 
> Currently scsi_target_emulate_report_luns iterates over the child device list
> twice, and there is no guarantee that this list is the same in both iterations.
> 
> The reason for iterating twice is that the first iteration calculates
> how much memory to allocate.  However if we use a dynamic array we can
> avoid iterating twice, and therefore we avoid this race.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1866707
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <20200913160259.32145-10-mlevitsk@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi/scsi-bus.c | 68 ++++++++++++++++++++++------------------------
>  1 file changed, 33 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index eda8cb7e70..b901e701f0 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -438,19 +438,23 @@ struct SCSITargetReq {
>  static void store_lun(uint8_t *outbuf, int lun)
>  {
>      if (lun < 256) {
> +        /* Simple logical unit addressing method*/
> +        outbuf[0] = 0;
>          outbuf[1] = lun;
> -        return;
> +    } else {
> +        /* Flat space addressing method */
> +        outbuf[0] = 0x40 | (lun >> 8);
> +        outbuf[1] = (lun & 255);
>      }
> -    outbuf[1] = (lun & 255);
> -    outbuf[0] = (lun >> 8) | 0x40;
>  }
>  
>  static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
>  {
>      BusChild *kid;
> -    int i, len, n;
>      int channel, id;
> -    bool found_lun0;
> +    uint8_t tmp[8] = {0};
> +    int len = 0;
> +    GByteArray *buf;
>  
>      if (r->req.cmd.xfer < 16) {
>          return false;
> @@ -458,46 +462,40 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
>      if (r->req.cmd.buf[2] > 2) {
>          return false;
>      }
> +
> +    /* reserve space for 63 LUNs*/
> +    buf = g_byte_array_sized_new(512);
> +
>      channel = r->req.dev->channel;
>      id = r->req.dev->id;
> -    found_lun0 = false;
> -    n = 0;
>  
> -    RCU_READ_LOCK_GUARD();
> +    /* add size (will be updated later to correct value */
My mistake here - I forgot closing ')' - as I said, there will
be significantly less issues like that in my patches soon.
> +    g_byte_array_append(buf, tmp, 8);
> +    len += 8;
>  
> -    QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
> -        DeviceState *qdev = kid->child;
> -        SCSIDevice *dev = SCSI_DEVICE(qdev);
> +    /* add LUN0 */
> +    g_byte_array_append(buf, tmp, 8);
> +    len += 8;
>  
> -        if (dev->channel == channel && dev->id == id) {
> -            if (dev->lun == 0) {
> -                found_lun0 = true;
> +    WITH_RCU_READ_LOCK_GUARD() {
> +        QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
> +            DeviceState *qdev = kid->child;
> +            SCSIDevice *dev = SCSI_DEVICE(qdev);
> +
> +            if (dev->channel == channel && dev->id == id && dev->lun != 0) {
> +                store_lun(tmp, dev->lun);
> +                g_byte_array_append(buf, tmp, 8);
> +                len += 8;
>              }
> -            n += 8;
>          }
>      }
> -    if (!found_lun0) {
> -        n += 8;
> -    }
> -
> -    scsi_target_alloc_buf(&r->req, n + 8);
> -
> -    len = MIN(n + 8, r->req.cmd.xfer & ~7);
> -    memset(r->buf, 0, len);
> -    stl_be_p(&r->buf[0], n);
> -    i = found_lun0 ? 8 : 16;
> -    QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
> -        DeviceState *qdev = kid->child;
> -        SCSIDevice *dev = SCSI_DEVICE(qdev);
>  
> -        if (dev->channel == channel && dev->id == id) {
> -            store_lun(&r->buf[i], dev->lun);
> -            i += 8;
> -        }
> -    }
> +    r->buf_len = len;
> +    r->buf = g_byte_array_free(buf, FALSE);
> +    r->len = MIN(len, r->req.cmd.xfer & ~7);
>  
> -    assert(i == n + 8);
> -    r->len = len;
> +    /* store the LUN list length */
> +    stl_be_p(&r->buf[0], len - 8);
>      return true;
>  }
>  
No doubt that I will RCU_READ_LOCK_GUARD more from now on.

Best regards,
	Maxim Levitsky
diff mbox series

Patch

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index eda8cb7e70..b901e701f0 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -438,19 +438,23 @@  struct SCSITargetReq {
 static void store_lun(uint8_t *outbuf, int lun)
 {
     if (lun < 256) {
+        /* Simple logical unit addressing method*/
+        outbuf[0] = 0;
         outbuf[1] = lun;
-        return;
+    } else {
+        /* Flat space addressing method */
+        outbuf[0] = 0x40 | (lun >> 8);
+        outbuf[1] = (lun & 255);
     }
-    outbuf[1] = (lun & 255);
-    outbuf[0] = (lun >> 8) | 0x40;
 }
 
 static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
 {
     BusChild *kid;
-    int i, len, n;
     int channel, id;
-    bool found_lun0;
+    uint8_t tmp[8] = {0};
+    int len = 0;
+    GByteArray *buf;
 
     if (r->req.cmd.xfer < 16) {
         return false;
@@ -458,46 +462,40 @@  static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
     if (r->req.cmd.buf[2] > 2) {
         return false;
     }
+
+    /* reserve space for 63 LUNs*/
+    buf = g_byte_array_sized_new(512);
+
     channel = r->req.dev->channel;
     id = r->req.dev->id;
-    found_lun0 = false;
-    n = 0;
 
-    RCU_READ_LOCK_GUARD();
+    /* add size (will be updated later to correct value */
+    g_byte_array_append(buf, tmp, 8);
+    len += 8;
 
-    QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
-        DeviceState *qdev = kid->child;
-        SCSIDevice *dev = SCSI_DEVICE(qdev);
+    /* add LUN0 */
+    g_byte_array_append(buf, tmp, 8);
+    len += 8;
 
-        if (dev->channel == channel && dev->id == id) {
-            if (dev->lun == 0) {
-                found_lun0 = true;
+    WITH_RCU_READ_LOCK_GUARD() {
+        QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
+            DeviceState *qdev = kid->child;
+            SCSIDevice *dev = SCSI_DEVICE(qdev);
+
+            if (dev->channel == channel && dev->id == id && dev->lun != 0) {
+                store_lun(tmp, dev->lun);
+                g_byte_array_append(buf, tmp, 8);
+                len += 8;
             }
-            n += 8;
         }
     }
-    if (!found_lun0) {
-        n += 8;
-    }
-
-    scsi_target_alloc_buf(&r->req, n + 8);
-
-    len = MIN(n + 8, r->req.cmd.xfer & ~7);
-    memset(r->buf, 0, len);
-    stl_be_p(&r->buf[0], n);
-    i = found_lun0 ? 8 : 16;
-    QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
-        DeviceState *qdev = kid->child;
-        SCSIDevice *dev = SCSI_DEVICE(qdev);
 
-        if (dev->channel == channel && dev->id == id) {
-            store_lun(&r->buf[i], dev->lun);
-            i += 8;
-        }
-    }
+    r->buf_len = len;
+    r->buf = g_byte_array_free(buf, FALSE);
+    r->len = MIN(len, r->req.cmd.xfer & ~7);
 
-    assert(i == n + 8);
-    r->len = len;
+    /* store the LUN list length */
+    stl_be_p(&r->buf[0], len - 8);
     return true;
 }