From 31e9e3691789469b93a75d0221387bab3e526094 Mon Sep 17 00:00:00 2001

From: Stefan Hajnoczi <stefanha@redhat.com>

Date: Tue, 21 Feb 2023 16:22:18 -0500

Subject: [PATCH 13/13] virtio-scsi: reset SCSI devices from main loop thread

RH-Author: Stefan Hajnoczi <stefanha@redhat.com>

RH-MergeRequest: 264: scsi: protect req->aiocb with AioContext lock

RH-Bugzilla: 2090990

RH-Acked-by: Stefano Garzarella <sgarzare@redhat.com>

RH-Acked-by: Miroslav Rezanina <mrezanin@redhat.com>

RH-Acked-by: Kevin Wolf <kwolf@redhat.com>

RH-Commit: [3/3] 30d7c2bd868efa6694992e75ace22fb48aef161b

When an IOThread is configured, the ctrl virtqueue is processed in the

IOThread. TMFs that reset SCSI devices are currently called directly

from the IOThread and trigger an assertion failure in blk_drain() from

the following call stack:

virtio_scsi_handle_ctrl_req -> virtio_scsi_do_tmf -> device_code_reset

-> scsi_disk_reset -> scsi_device_purge_requests -> blk_drain

  ../block/block-backend.c:1780: void blk_drain(BlockBackend *): Assertion `qemu_in_main_thread()' failed.

The blk_drain() function is not designed to be called from an IOThread

because it needs the Big QEMU Lock (BQL).

This patch defers TMFs that reset SCSI devices to a Bottom Half (BH)

that runs in the main loop thread under the BQL. This way it's safe to

call blk_drain() and the assertion failure is avoided.

Introduce s->tmf_bh_list for tracking TMF requests that have been

deferred to the BH. When the BH runs it will grab the entire list and

process all requests. Care must be taken to clear the list when the

virtio-scsi device is reset or unrealized. Otherwise deferred TMF

requests could execute later and lead to use-after-free or other

undefined behavior.

The s->resetting counter that's used by TMFs that reset SCSI devices is

accessed from multiple threads. This patch makes that explicit by using

atomic accessor functions. With this patch applied the counter is only

modified by the main loop thread under the BQL but can be read by any

thread.

Reported-by: Qing Wang <qinwang@redhat.com>

Cc: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Message-Id: <20230221212218.1378734-4-stefanha@redhat.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

(cherry picked from commit be2c42b97c3a3a395b2f05bad1b6c7de20ecf2a5)

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

Conflicts:

- hw/scsi/virtio-scsi.c

  - VirtIOSCSIReq is defined in include/hw/virtio/virtio-scsi.h

    downstream instead of hw/scsi/virtio-scsi.c because commit

    3dc584abeef0 ("virtio-scsi: move request-related items from .h to

    .c") is missing. Update the struct fields in virtio-scsi.h

    downstream.

  - Use qbus_reset_all() downstream instead of bus_cold_reset() because

    commit 4a5fc890b1d3 ("scsi: Use device_cold_reset() and

    bus_cold_reset()") is missing.

  - Drop GLOBAL_STATE_CODE() because these macros don't exist

    downstream. They are assertions/documentation and can be removed

    without affecting the code.

---

 hw/scsi/virtio-scsi.c           | 155 +++++++++++++++++++++++++-------

 include/hw/virtio/virtio-scsi.h |  21 +++--

 2 files changed, 139 insertions(+), 37 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c

index a35257c35a..ef19a9bcd0 100644

--- a/hw/scsi/virtio-scsi.c

+++ b/hw/scsi/virtio-scsi.c

@@ -256,6 +256,118 @@ static inline void virtio_scsi_ctx_check(VirtIOSCSI *s, SCSIDevice *d)

     }

 }

+static void virtio_scsi_do_one_tmf_bh(VirtIOSCSIReq *req)

+{

+    VirtIOSCSI *s = req->dev;

+    SCSIDevice *d = virtio_scsi_device_get(s, req->req.tmf.lun);

+    BusChild *kid;

+    int target;

+

+    switch (req->req.tmf.subtype) {

+    case VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET:

+        if (!d) {

+            req->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET;

+            goto out;

+        }

+        if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {

+            req->resp.tmf.response = VIRTIO_SCSI_S_INCORRECT_LUN;

+            goto out;

+        }

+        qatomic_inc(&s->resetting);

+        qdev_reset_all(&d->qdev);

+        qatomic_dec(&s->resetting);

+        break;

+

+    case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:

+        target = req->req.tmf.lun[1];

+        qatomic_inc(&s->resetting);

+

+        rcu_read_lock();

+        QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) {

+            SCSIDevice *d1 = SCSI_DEVICE(kid->child);

+            if (d1->channel == 0 && d1->id == target) {

+                qdev_reset_all(&d1->qdev);

+            }

+        }

+        rcu_read_unlock();

+

+        qatomic_dec(&s->resetting);

+        break;

+

+    default:

+        g_assert_not_reached();

+        break;

+    }

+

+out:

+    object_unref(OBJECT(d));

+

+    virtio_scsi_acquire(s);

+    virtio_scsi_complete_req(req);

+    virtio_scsi_release(s);

+}

+

+/* Some TMFs must be processed from the main loop thread */

+static void virtio_scsi_do_tmf_bh(void *opaque)

+{

+    VirtIOSCSI *s = opaque;

+    QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);

+    VirtIOSCSIReq *req;

+    VirtIOSCSIReq *tmp;

+

+    virtio_scsi_acquire(s);

+

+    QTAILQ_FOREACH_SAFE(req, &s->tmf_bh_list, next, tmp) {

+        QTAILQ_REMOVE(&s->tmf_bh_list, req, next);

+        QTAILQ_INSERT_TAIL(&reqs, req, next);

+    }

+

+    qemu_bh_delete(s->tmf_bh);

+    s->tmf_bh = NULL;

+

+    virtio_scsi_release(s);

+

+    QTAILQ_FOREACH_SAFE(req, &reqs, next, tmp) {

+        QTAILQ_REMOVE(&reqs, req, next);

+        virtio_scsi_do_one_tmf_bh(req);

+    }

+}

+

+static void virtio_scsi_reset_tmf_bh(VirtIOSCSI *s)

+{

+    VirtIOSCSIReq *req;

+    VirtIOSCSIReq *tmp;

+

+    virtio_scsi_acquire(s);

+

+    if (s->tmf_bh) {

+        qemu_bh_delete(s->tmf_bh);

+        s->tmf_bh = NULL;

+    }

+

+    QTAILQ_FOREACH_SAFE(req, &s->tmf_bh_list, next, tmp) {

+        QTAILQ_REMOVE(&s->tmf_bh_list, req, next);

+

+        /* SAM-6 6.3.2 Hard reset */

+        req->resp.tmf.response = VIRTIO_SCSI_S_TARGET_FAILURE;

+        virtio_scsi_complete_req(req);

+    }

+

+    virtio_scsi_release(s);

+}

+

+static void virtio_scsi_defer_tmf_to_bh(VirtIOSCSIReq *req)

+{

+    VirtIOSCSI *s = req->dev;

+

+    QTAILQ_INSERT_TAIL(&s->tmf_bh_list, req, next);

+

+    if (!s->tmf_bh) {

+        s->tmf_bh = qemu_bh_new(virtio_scsi_do_tmf_bh, s);

+        qemu_bh_schedule(s->tmf_bh);

+    }

+}

+

 /* Return 0 if the request is ready to be completed and return to guest;

  * -EINPROGRESS if the request is submitted and will be completed later, in the

  *  case of async cancellation. */

@@ -263,8 +375,6 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)

 {

     SCSIDevice *d = virtio_scsi_device_get(s, req->req.tmf.lun);

     SCSIRequest *r, *next;

-    BusChild *kid;

-    int target;

     int ret = 0;

     virtio_scsi_ctx_check(s, d);

@@ -321,15 +431,9 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)

         break;

     case VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET:

-        if (!d) {

-            goto fail;

-        }

-        if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {

-            goto incorrect_lun;

-        }

-        s->resetting++;

-        qdev_reset_all(&d->qdev);

-        s->resetting--;

+    case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:

+        virtio_scsi_defer_tmf_to_bh(req);

+        ret = -EINPROGRESS;

         break;

     case VIRTIO_SCSI_T_TMF_ABORT_TASK_SET:

@@ -372,22 +476,6 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)

         }

         break;

-    case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:

-        target = req->req.tmf.lun[1];

-        s->resetting++;

-

-        rcu_read_lock();

-        QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) {

-            SCSIDevice *d1 = SCSI_DEVICE(kid->child);

-            if (d1->channel == 0 && d1->id == target) {

-                qdev_reset_all(&d1->qdev);

-            }

-        }

-        rcu_read_unlock();

-

-        s->resetting--;

-        break;

-

     case VIRTIO_SCSI_T_TMF_CLEAR_ACA:

     default:

         req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_REJECTED;

@@ -603,7 +691,7 @@ static void virtio_scsi_request_cancelled(SCSIRequest *r)

     if (!req) {

         return;

     }

-    if (req->dev->resetting) {

+    if (qatomic_read(&req->dev->resetting)) {

         req->resp.cmd.response = VIRTIO_SCSI_S_RESET;

     } else {

         req->resp.cmd.response = VIRTIO_SCSI_S_ABORTED;

@@ -784,9 +872,12 @@ static void virtio_scsi_reset(VirtIODevice *vdev)

     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);

     assert(!s->dataplane_started);

-    s->resetting++;

+

+    virtio_scsi_reset_tmf_bh(s);

+

+    qatomic_inc(&s->resetting);

     qbus_reset_all(BUS(&s->bus));

-    s->resetting--;

+    qatomic_dec(&s->resetting);

     vs->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;

     vs->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;

@@ -1018,6 +1109,8 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)

     VirtIOSCSI *s = VIRTIO_SCSI(dev);

     Error *err = NULL;

+    QTAILQ_INIT(&s->tmf_bh_list);

+

     virtio_scsi_common_realize(dev,

                                virtio_scsi_handle_ctrl,

                                virtio_scsi_handle_event,

@@ -1055,6 +1148,8 @@ static void virtio_scsi_device_unrealize(DeviceState *dev)

 {

     VirtIOSCSI *s = VIRTIO_SCSI(dev);

+    virtio_scsi_reset_tmf_bh(s);

+

     qbus_set_hotplug_handler(BUS(&s->bus), NULL);

     virtio_scsi_common_unrealize(dev);

 }

diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h

index 543681bc18..b0e36f25aa 100644

--- a/include/hw/virtio/virtio-scsi.h

+++ b/include/hw/virtio/virtio-scsi.h

@@ -77,13 +77,22 @@ struct VirtIOSCSICommon {

     VirtQueue **cmd_vqs;

 };

+struct VirtIOSCSIReq;

+

 struct VirtIOSCSI {

     VirtIOSCSICommon parent_obj;

     SCSIBus bus;

-    int resetting;

+    int resetting; /* written from main loop thread, read from any thread */

     bool events_dropped;

+    /*

+     * TMFs deferred to main loop BH. These fields are protected by

+     * virtio_scsi_acquire().

+     */

+    QEMUBH *tmf_bh;

+    QTAILQ_HEAD(, VirtIOSCSIReq) tmf_bh_list;

+

     /* Fields for dataplane below */

     AioContext *ctx; /* one iothread per virtio-scsi-pci for now */

@@ -106,13 +115,11 @@ typedef struct VirtIOSCSIReq {

     QEMUSGList qsgl;

     QEMUIOVector resp_iov;

-    union {

-        /* Used for two-stage request submission */

-        QTAILQ_ENTRY(VirtIOSCSIReq) next;

+    /* Used for two-stage request submission and TMFs deferred to BH */

+    QTAILQ_ENTRY(VirtIOSCSIReq) next;

-        /* Used for cancellation of request during TMFs */

-        int remaining;

-    };

+    /* Used for cancellation of request during TMFs */

+    int remaining;

     SCSIRequest *sreq;

     size_t resp_size;

-- 

2.37.3