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

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

[Jia Jia]

vhost_scsi_send_evt() is called with the event virtqueue mutex held.
If the worker is gone, the fallback currently calls
vhost_scsi_complete_events(), which tries to acquire the same mutex again
and deadlocks the caller.

Split event completion into a helper for callers that already hold the
mutex and a locking wrapper for the event worker. Use the helper on the
fallback path.

Link: https://lore.kernel.org/all/20260905005352.1E5B01F00A3D@smtp.kernel.org/
Fixes: b1b2ce58ed23 ("vhost-scsi: Handle vhost_vq_work_queue failures for events")
Signed-off-by: Jia Jia <physicalmtea@gmail.com>
---
Sashiko AI flagged this while reviewing
the vhost-scsi event queue fix.
This is a pre-existing self-deadlock. It was reproduced in a follow-up test.

Trigger: vq->worker == NULL. vhost_vq_work_queue() then returns false,
and a subsequent vhost_scsi_do_plug() call deadlocks. I do not know what
normal condition gets us here; the normal vhost-scsi worker detach/reset
paths do not reach this code. The only reproduction I could come up with
was killing the vhost-scsi worker. This still looks like a low-probability
condition.

 drivers/vhost/scsi.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 7a1f39a32..7c905e9ad 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -630,19 +630,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) {
 		if (!drop)
 			vhost_scsi_do_evt_work(vs, evt);
 		vhost_scsi_free_evt(vs, evt);
 	}
+}
+
+static void vhost_scsi_complete_events(struct vhost_scsi *vs, bool drop)
+{
+	struct vhost_virtqueue *vq = &vs->vqs[VHOST_SCSI_VQ_EVT].vq;
+
+	mutex_lock(&vq->mutex);
+	__vhost_scsi_complete_events(vs, drop);
 	mutex_unlock(&vq->mutex);
 }
 
@@ -1829,7 +1836,7 @@ vhost_scsi_send_evt(struct vhost_scsi *vs, struct vhost_virtqueue *vq,
 
 	llist_add(&evt->list, &vs->vs_event_list);
 	if (!vhost_vq_work_queue(vq, &vs->vs_event_work))
-		vhost_scsi_complete_events(vs, true);
+		__vhost_scsi_complete_events(vs, true);
 }
 
 static void vhost_scsi_evt_handle_kick(struct vhost_work *work)

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

[sashiko-bot]

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 <physicalmtea@gmail.com>

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/

> 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.

>  		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.

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?

>  		vhost_scsi_free_evt(vs, evt);
>  	}
> +}

[ ... ]

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