Re: [PATCH] vhost-scsi: do not relock event vq mutex on send_evt fallback

On Tue, Sep 15, 2026 at 09:30:27PM +0800, Jia Jia wrote:

quoted

On Mon, Sep 14, 2026 at 04:38:57PM +0800, Jia Jia wrote:

quoted

quoted

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] vhost_scsi_do_evt_work() accesses vq->iov[out] without verifying that the guest actually provided an input buffer, leading to arbitrary user-space memory corruption.
- [High] Events are processed in LIFO order instead of FIFO, which can cause state corruption for hotplug/hotunplug sequences.
--

commit 05830c30fc2e4dc682c270a9b487ae5abb66218b
Author: Jia Jia [off-list ref]

vhost-scsi: do not relock event vq mutex on send_evt fallback

This commit resolves a deadlock during fallback event handling when the vhost
worker has disconnected. It splits event completion into an internal unlocked
helper and a locked wrapper to safely reuse the completion path.

Link: https://lore.kernel.org/all/20260905005352.1E5B01F00A3D@smtp.kernel.org/ (local)

quoted

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 4f8c0260bc9e7..42abf18291b75 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c

@@ -660,19 +660,26 @@ vhost_scsi_do_evt_work(struct vhost_scsi *vs, struct vhost_scsi_evt *evt)
      vhost_scsi_log_write(vq, vq_log, log_num);
 }

-static void vhost_scsi_complete_events(struct vhost_scsi *vs, bool drop)
+/* Caller must hold the event virtqueue mutex. */
+static void __vhost_scsi_complete_events(struct vhost_scsi *vs, bool drop)
 {
-     struct vhost_virtqueue *vq = &vs->vqs[VHOST_SCSI_VQ_EVT].vq;
      struct vhost_scsi_evt *evt, *t;
      struct llist_node *llnode;

-     mutex_lock(&vq->mutex);
      llnode = llist_del_all(&vs->vs_event_list);
      llist_for_each_entry_safe(evt, t, llnode, list) {

[Severity: High]
This is a pre-existing issue, but does this loop deliver events to the guest
in reverse order?

The events are added to vs_event_list using llist_add(), which constructs a
last-in, first-out list. Because lnode is retrieved locklessly with
llist_del_all(), and the iteration happens without first calling
llist_reverse_order(), the events appear to be processed from newest to
oldest.

Could this cause state corruption for hotplug and hotunplug sequences? For
example, if a device is plugged and then rapidly unplugged, the guest might
receive the unplug event first and the plug event second, leaving it
erroneously believing the device is still present.

Looks like this could get the acquire order wrong? Need to verify.

didn't look into this yet - did u reach any conclusion?

Already reproduced this using a test program. To hit what Sashiko AI described,
need multiple vhost_scsi_send_evt->llist_add (at least 2) before the
worker schedules
vhost_scsi_complete_events(false).

I personally think this is not very easy to hit, first this kind of
hotplug/unplug event is not very frequent,
even if there are multiple calls, as long as the worker is idle it
will run vhost_scsi_complete_events(false)
quickly, so pending is often only 1.

So to reproduce, I created multiple LUNs, and concurrently called 8 ln
-s (only triggers RESCAN,
no unplug, extra LUNs are just so we can see the lun numbers better).
This increases the chance that
vhost_scsi_send_evt races in before vhost_scsi_complete_events(false)
runs, so there are at least 2+ pending llist.
Finally when vhost_scsi_complete_events(false) runs, the guest sees
the later-linked LUN first.
(before testing I thought this needed a lot of SCSI I/O to reproduce,
actually it doesn't)
I only tested plug, the guest did see add LUN order reversed.

okay... so llist_reverse_order ? Will that fix it?

quoted

quoted

quoted

quoted

              if (!drop)
                      vhost_scsi_do_evt_work(vs, evt);

[Severity: Critical]
This is a pre-existing issue, but can this function access out-of-bounds or
stale memory if the guest provides an empty input buffer?

Looking at vhost_scsi_do_evt_work() in drivers/vhost/scsi.c, the code uses
vhost_get_vq_desc() which populates vq->iov:

    head = vhost_get_vq_desc(vq, vq->iov,
            ARRAY_SIZE(vq->iov), &out, &in,
            vq_log, &log_num);
    ...
    if ((vq->iov[out].iov_len != sizeof(struct virtio_scsi_event))) {
        vq_err(vq, "Expecting virtio_scsi_event, got %zu bytes\n",
                vq->iov[out].iov_len);

If a guest submits a descriptor chain with only read-only descriptors,
vhost_get_vq_desc() sets in = 0. The out variable holds the count of
read-only descriptors, meaning the first writable descriptor expected by the
host would be at index out.

This is a dup -- an earlier patch already fixed it:
https://lore.kernel.org/all/20260904005240.502787-1-physicalmtea@gmail.com/ (local)
It was while fixing that earlier issue that Sashiko spotted this
adjacent problem.

quoted

Does the length check above access vq->iov[out].iov_len without verifying
that in > 0?

If in is 0, vq->iov[out] was not populated by vhost_get_vq_desc() and
contains stale data from a previous request. Could a malicious guest use
this to force the kernel to write the event structure to a previously
legitimate but now hot-unplugged host virtual address via the
__copy_to_user() call later in the function?

quoted

              vhost_scsi_free_evt(vs, evt);
      }
+}

[ ... ]

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260912090420.47439-1-physicalmtea@gmail.com?part=1