[PATCH v2 7/7] virtio-scsi: use scsi_device_get

Maxim Levitsky posted 7 patches 1 year, 5 months ago

[PATCH v2 7/7] virtio-scsi: use scsi_device_get

Posted by Maxim Levitsky 1 year, 5 months ago
This will help us to avoid the scsi device disappearing
after we took a reference to it.

It doesn't by itself forbid case when we try to access
an unrealized device

Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 hw/scsi/virtio-scsi.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 1cc1fc557c..65cd4186fe 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -33,7 +33,7 @@ static inline int virtio_scsi_get_lun(uint8_t *lun)
     return ((lun[2] << 8) | lun[3]) & 0x3FFF;
 }
 
-static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
+static inline SCSIDevice *virtio_scsi_device_get(VirtIOSCSI *s, uint8_t *lun)
 {
     SCSIDevice *device = NULL;
 
@@ -44,7 +44,7 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
         return NULL;
     }
 
-    device = scsi_device_find(&s->bus, 0, lun[1], virtio_scsi_get_lun(lun));
+    device = scsi_device_get(&s->bus, 0, lun[1], virtio_scsi_get_lun(lun));
 
     /*
      * This function might run on the IO thread and we might race against
@@ -61,6 +61,8 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
     return device;
 }
 
+
+
 void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
@@ -273,7 +275,7 @@ static inline void virtio_scsi_ctx_check(VirtIOSCSI *s, SCSIDevice *d)
  *  case of async cancellation. */
 static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
 {
-    SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf.lun);
+    SCSIDevice *d = virtio_scsi_device_get(s, req->req.tmf.lun);
     SCSIRequest *r, *next;
     BusChild *kid;
     int target;
@@ -387,10 +389,10 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
 
         rcu_read_lock();
         QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) {
-             d = SCSI_DEVICE(kid->child);
-             if (d->channel == 0 && d->id == target) {
-                qdev_reset_all(&d->qdev);
-             }
+            SCSIDevice *d1 = SCSI_DEVICE(kid->child);
+            if (d1->channel == 0 && d1->id == target) {
+                qdev_reset_all(&d1->qdev);
+            }
         }
         rcu_read_unlock();
 
@@ -403,14 +405,17 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
         break;
     }
 
+    object_unref(OBJECT(d));
     return ret;
 
 incorrect_lun:
     req->resp.tmf.response = VIRTIO_SCSI_S_INCORRECT_LUN;
+    object_unref(OBJECT(d));
     return ret;
 
 fail:
     req->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET;
+    object_unref(OBJECT(d));
     return ret;
 }
 
@@ -581,7 +586,7 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
         }
     }
 
-    d = virtio_scsi_device_find(s, req->req.cmd.lun);
+    d = virtio_scsi_device_get(s, req->req.cmd.lun);
     if (!d) {
         req->resp.cmd.response = VIRTIO_SCSI_S_BAD_TARGET;
         virtio_scsi_complete_cmd_req(req);
@@ -597,10 +602,12 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
             req->sreq->cmd.xfer > req->qsgl.size)) {
         req->resp.cmd.response = VIRTIO_SCSI_S_OVERRUN;
         virtio_scsi_complete_cmd_req(req);
+        object_unref(OBJECT(d));
         return -ENOBUFS;
     }
     scsi_req_ref(req->sreq);
     blk_io_plug(d->conf.blk);
+    object_unref(OBJECT(d));
     return 0;
 }
 
-- 
2.17.2


Re: [PATCH v2 7/7] virtio-scsi: use scsi_device_get

Posted by Stefan Hajnoczi 1 year, 4 months ago
On Mon, May 11, 2020 at 07:09:51PM +0300, Maxim Levitsky wrote:
> This will help us to avoid the scsi device disappearing
> after we took a reference to it.
> 
> It doesn't by itself forbid case when we try to access
> an unrealized device
> 
> Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  hw/scsi/virtio-scsi.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [PATCH v2 7/7] virtio-scsi: use scsi_device_get

Posted by Stefan Hajnoczi 1 year, 4 months ago
On Mon, May 11, 2020 at 07:09:51PM +0300, Maxim Levitsky wrote:
> This will help us to avoid the scsi device disappearing
> after we took a reference to it.
> 
> It doesn't by itself forbid case when we try to access
> an unrealized device
> 
> Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  hw/scsi/virtio-scsi.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)

I'm not very familiar with the SCSI emulation code, but this looks
correct. My understanding of what this patch does:

This patch keeps SCSIDevice alive between scsi_device_find() and
scsi_req_new(). Previously no SCSIDevice ref was taken so the device
could have been freed before scsi_req_new() had a chance to take a ref.

The TMF case is similar: the SCSIDevice ref must be held during
virtio_scsi_do_tmf(). We don't need to worry about the async cancel
notifiers because the request being canceled already holds a ref.

Re: [PATCH v2 7/7] virtio-scsi: use scsi_device_get

Posted by Maxim Levitsky 1 year, 3 months ago
On Wed, 2020-05-27 at 16:50 +0100, Stefan Hajnoczi wrote:
> On Mon, May 11, 2020 at 07:09:51PM +0300, Maxim Levitsky wrote:
> > This will help us to avoid the scsi device disappearing
> > after we took a reference to it.
> > 
> > It doesn't by itself forbid case when we try to access
> > an unrealized device
> > 
> > Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  hw/scsi/virtio-scsi.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> I'm not very familiar with the SCSI emulation code, but this looks
> correct. My understanding of what this patch does:
> 
> This patch keeps SCSIDevice alive between scsi_device_find() and
> scsi_req_new(). Previously no SCSIDevice ref was taken so the device
> could have been freed before scsi_req_new() had a chance to take a ref.
Yep, I also verified now that this is what happens.

> 
> The TMF case is similar: the SCSIDevice ref must be held during
> virtio_scsi_do_tmf(). We don't need to worry about the async cancel
> notifiers because the request being canceled already holds a ref.
This code I understand less to be honest, but in the worst case,
the patch shoudn't make it worse.

Best regards,
	Maxim Levitsky