From 4382f167cfbc21a49424e6dd9347681008363128 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 16 May 2023 15:02:21 -0400 Subject: [PATCH] virtio-scsi: avoid race between unplug and transport event Only report a transport reset event to the guest after the SCSIDevice has been unrealized by qdev_simple_device_unplug_cb(). qdev_simple_device_unplug_cb() sets the SCSIDevice's qdev.realized field to false so that scsi_device_find/get() no longer see it. scsi_target_emulate_report_luns() also needs to be updated to filter out SCSIDevices that are unrealized. Change virtio_scsi_push_event() to take event information as an argument instead of the SCSIDevice. This allows virtio_scsi_hotunplug() to emit a VIRTIO_SCSI_T_TRANSPORT_RESET event after the SCSIDevice has already been unrealized. These changes ensure that the guest driver does not see the SCSIDevice that's being unplugged if it responds very quickly to the transport reset event. Reviewed-by: Paolo Bonzini Reviewed-by: Michael S. Tsirkin Reviewed-by: Daniil Tatianin Signed-off-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf Message-Id: <20230516190238.8401-4-stefanha@redhat.com> Signed-off-by: Kevin Wolf --- hw/scsi/scsi-bus.c | 3 +- hw/scsi/virtio-scsi.c | 86 ++++++++++++++++++++++++++++++++++++--------------- 2 files changed, 63 insertions(+), 26 deletions(-) diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index 8857ff41f6..64013c8a24 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -487,7 +487,8 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r) DeviceState *qdev = kid->child; SCSIDevice *dev = SCSI_DEVICE(qdev); - if (dev->channel == channel && dev->id == id && dev->lun != 0) { + if (dev->channel == channel && dev->id == id && dev->lun != 0 && + qdev_is_realized(&dev->qdev)) { store_lun(tmp, dev->lun); g_byte_array_append(buf, tmp, 8); len += 8; diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index 612c525d9d..ae314af3de 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -933,13 +933,27 @@ static void virtio_scsi_reset(VirtIODevice *vdev) s->events_dropped = false; } -static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev, - uint32_t event, uint32_t reason) +typedef struct { + uint32_t event; + uint32_t reason; + union { + /* Used by messages specific to a device */ + struct { + uint32_t id; + uint32_t lun; + } address; + }; +} VirtIOSCSIEventInfo; + +static void virtio_scsi_push_event(VirtIOSCSI *s, + const VirtIOSCSIEventInfo *info) { VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s); VirtIOSCSIReq *req; VirtIOSCSIEvent *evt; VirtIODevice *vdev = VIRTIO_DEVICE(s); + uint32_t event = info->event; + uint32_t reason = info->reason; if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) { return; @@ -965,27 +979,28 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev, memset(evt, 0, sizeof(VirtIOSCSIEvent)); evt->event = virtio_tswap32(vdev, event); evt->reason = virtio_tswap32(vdev, reason); - if (!dev) { - assert(event == VIRTIO_SCSI_T_EVENTS_MISSED); - } else { + if (event != VIRTIO_SCSI_T_EVENTS_MISSED) { evt->lun[0] = 1; - evt->lun[1] = dev->id; + evt->lun[1] = info->address.id; /* Linux wants us to keep the same encoding we use for REPORT LUNS. */ - if (dev->lun >= 256) { - evt->lun[2] = (dev->lun >> 8) | 0x40; + if (info->address.lun >= 256) { + evt->lun[2] = (info->address.lun >> 8) | 0x40; } - evt->lun[3] = dev->lun & 0xFF; + evt->lun[3] = info->address.lun & 0xFF; } trace_virtio_scsi_event(virtio_scsi_get_lun(evt->lun), event, reason); - + virtio_scsi_complete_req(req); } static void virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq) { if (s->events_dropped) { - virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0); + VirtIOSCSIEventInfo info = { + .event = VIRTIO_SCSI_T_NO_EVENT, + }; + virtio_scsi_push_event(s, &info); } } @@ -1009,9 +1024,17 @@ static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense) if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_CHANGE) && dev->type != TYPE_ROM) { + VirtIOSCSIEventInfo info = { + .event = VIRTIO_SCSI_T_PARAM_CHANGE, + .reason = sense.asc | (sense.ascq << 8), + .address = { + .id = dev->id, + .lun = dev->lun, + }, + }; + virtio_scsi_acquire(s); - virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_PARAM_CHANGE, - sense.asc | (sense.ascq << 8)); + virtio_scsi_push_event(s, &info); virtio_scsi_release(s); } } @@ -1046,10 +1069,17 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, } if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { + VirtIOSCSIEventInfo info = { + .event = VIRTIO_SCSI_T_TRANSPORT_RESET, + .reason = VIRTIO_SCSI_EVT_RESET_RESCAN, + .address = { + .id = sd->id, + .lun = sd->lun, + }, + }; + virtio_scsi_acquire(s); - virtio_scsi_push_event(s, sd, - VIRTIO_SCSI_T_TRANSPORT_RESET, - VIRTIO_SCSI_EVT_RESET_RESCAN); + virtio_scsi_push_event(s, &info); scsi_bus_set_ua(&s->bus, SENSE_CODE(REPORTED_LUNS_CHANGED)); virtio_scsi_release(s); } @@ -1062,15 +1092,14 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev, VirtIOSCSI *s = VIRTIO_SCSI(vdev); SCSIDevice *sd = SCSI_DEVICE(dev); AioContext *ctx = s->ctx ?: qemu_get_aio_context(); - - if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { - virtio_scsi_acquire(s); - virtio_scsi_push_event(s, sd, - VIRTIO_SCSI_T_TRANSPORT_RESET, - VIRTIO_SCSI_EVT_RESET_REMOVED); - scsi_bus_set_ua(&s->bus, SENSE_CODE(REPORTED_LUNS_CHANGED)); - virtio_scsi_release(s); - } + VirtIOSCSIEventInfo info = { + .event = VIRTIO_SCSI_T_TRANSPORT_RESET, + .reason = VIRTIO_SCSI_EVT_RESET_REMOVED, + .address = { + .id = sd->id, + .lun = sd->lun, + }, + }; aio_disable_external(ctx); qdev_simple_device_unplug_cb(hotplug_dev, dev, errp); @@ -1082,6 +1111,13 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev, blk_set_aio_context(sd->conf.blk, qemu_get_aio_context(), NULL); virtio_scsi_release(s); } + + if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { + virtio_scsi_acquire(s); + virtio_scsi_push_event(s, &info); + scsi_bus_set_ua(&s->bus, SENSE_CODE(REPORTED_LUNS_CHANGED)); + virtio_scsi_release(s); + } } static struct SCSIBusInfo virtio_scsi_scsi_info = { -- 2.11.0