[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 10/10] scsi/scsi_bus: fix races in REPORT LUNS
From: |
Maxim Levitsky |
Subject: |
Re: [PATCH 10/10] scsi/scsi_bus: fix races in REPORT LUNS |
Date: |
Wed, 30 Sep 2020 17:34:28 +0300 |
User-agent: |
Evolution 3.36.3 (3.36.3-1.fc32) |
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
- Re: [PATCH 05/10] device-core: use RCU for list of children of a bus, (continued)
- [PATCH 07/10] scsi/scsi-bus: scsi_device_find: don't return unrealized devices, Paolo Bonzini, 2020/09/25
- [PATCH 08/10] scsi/scsi_bus: Add scsi_device_get, Paolo Bonzini, 2020/09/25
- [PATCH 06/10] device-core: use atomic_set on .realized property, Paolo Bonzini, 2020/09/25
- [PATCH 04/10] device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add, Paolo Bonzini, 2020/09/25
- [PATCH 09/10] virtio-scsi: use scsi_device_get, Paolo Bonzini, 2020/09/25
- [PATCH 10/10] scsi/scsi_bus: fix races in REPORT LUNS, Paolo Bonzini, 2020/09/25
- Re: [PATCH 10/10] scsi/scsi_bus: fix races in REPORT LUNS,
Maxim Levitsky <=
- [PATCH 03/10] scsi/scsi_bus: switch search direction in scsi_device_find, Paolo Bonzini, 2020/09/25
- Re: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread, no-reply, 2020/09/25
- Re: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread, no-reply, 2020/09/25
- Re: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread, no-reply, 2020/09/25
- Re: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread, no-reply, 2020/09/25
- Re: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread, no-reply, 2020/09/25