| Message ID | 20200511160951.8733-8-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread | expand |
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.
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>
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
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; }
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(-)