[PATCH v1] vhost-scsi: do not validate vrings during endpoint teardown

[PATCH v1] vhost-scsi: do not validate vrings during endpoint teardown

[Yuho Choi]

vhost_scsi_clear_endpoint() checks every virtqueue with
vhost_vq_access_ok() before disabling the backends and waiting for
in-flight SCSI commands.  If that check fails, endpoint teardown is
skipped.

vhost_scsi_release() ignores the error and frees the vhost-scsi state.
vhost_dev_stop() drains vhost work, but it does not wait for commands
owned by the target core.  A later command or TMF completion can therefore
dereference its saved virtqueue pointer after the virtqueue has been freed.

Vring access is not a prerequisite for tearing an endpoint down.  Remove
the check so teardown always disables the backends and waits for in-flight
commands before releasing their state.

Fixes: 057cbf49a1f0 ("tcm_vhost: Initial merge for vhost level target fabric driver")
Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
---
 drivers/vhost/scsi.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index e44044449cef..f0966f25b09a 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -2194,17 +2194,10 @@ vhost_scsi_clear_endpoint(struct vhost_scsi *vs,
 	struct vhost_scsi_tpg *tpg;
 	struct vhost_virtqueue *vq;
 	bool match = false;
-	int index, ret, i;
+	int ret, i;
 	u8 target;
 
 	mutex_lock(&vs->dev.mutex);
-	/* Verify that ring has been setup correctly. */
-	for (index = 0; index < vs->dev.nvqs; ++index) {
-		if (!vhost_vq_access_ok(&vs->vqs[index].vq)) {
-			ret = -EFAULT;
-			goto err_dev;
-		}
-	}
 
 	if (!vs->vs_tpg) {
 		ret = 0;
-- 
2.43.0

Re: [PATCH v1] vhost-scsi: do not validate vrings during endpoint teardown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] vhost-scsi: unhandled vhost_vq_init_access() failure in
  vhost_scsi_set_endpoint() [1]
--

commit b696a8fa1d0b9b5e5c27a63e36f3d5e5c268720d
Author: Yuho Choi <oss.patchbox@gmail.com>

vhost-scsi: do not validate vrings during endpoint teardown

This commit removes the vhost_vq_access_ok() check from
vhost_scsi_clear_endpoint() so that endpoint teardown always disables the
virtqueue backends and waits for in-flight SCSI commands before state is
freed.

> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index e44044449cef8..f0966f25b09a9 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -2194,17 +2194,10 @@ vhost_scsi_clear_endpoint(struct vhost_scsi *vs,
>  	struct vhost_scsi_tpg *tpg;
>  	struct vhost_virtqueue *vq;
>  	bool match = false;
> -	int index, ret, i;
> +	int ret, i;
>  	u8 target;
>  
>  	mutex_lock(&vs->dev.mutex);
> -	/* Verify that ring has been setup correctly. */
> -	for (index = 0; index < vs->dev.nvqs; ++index) {
> -		if (!vhost_vq_access_ok(&vs->vqs[index].vq)) {
> -			ret = -EFAULT;
> -			goto err_dev;
> -		}
> -	}
>  
>  	if (!vs->vs_tpg) {
>  		ret = 0;

[Severity: High]
This problem wasn't introduced by this patch, but should the paired
vhost_scsi_set_endpoint() path check the return value of
vhost_vq_init_access() [1]?

drivers/vhost/scsi.c:vhost_scsi_set_endpoint() {
    ...
		for (i = 0; i < vs->dev.nvqs; i++) {
			vq = &vs->vqs[i].vq;
			if (i >= VHOST_SCSI_VQ_IO && !vs->vqs[i].scsi_cmds)
				continue;

			mutex_lock(&vq->mutex);
			vhost_vq_set_backend(vq, vs_tpg);
			vhost_vq_init_access(vq);
			mutex_unlock(&vq->mutex);
		}
    ...
}

If userspace invokes VHOST_SCSI_SET_ENDPOINT before configuring the vring
addresses, vhost_vq_init_access() fails for the control and event
virtqueues while their backends remain set to vs_tpg, leaving
last_used_idx uninitialized and breaking event delivery and control
command handling.

[1] https://sashiko.dev/#/bug/linux-00d9df6c-e264-4a85-ae70-5ede3bace73b

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106054811.281798-2-oss.patchbox@gmail.com?part=1

Re: [PATCH v1] vhost-scsi: do not validate vrings during endpoint teardown

[Mike Christie]

On 11/5/26 11:48 PM, Yuho Choi wrote:
> vhost_scsi_clear_endpoint() checks every virtqueue with vhost_vq_access_ok() before disabling the backends and waiting for in-flight SCSI commands. If that check fails, endpoint teardown is skipped. vhost_scsi_release() ignores the error and
> 
> 
> vhost_scsi_clear_endpoint() checks every virtqueue with
> vhost_vq_access_ok() before disabling the backends and waiting for
> in-flight SCSI commands.  If that check fails, endpoint teardown is
> skipped.
> 
> vhost_scsi_release() ignores the error and frees the vhost-scsi state.
> vhost_dev_stop() drains vhost work, but it does not wait for commands
> owned by the target core.  A later command or TMF completion can therefore
> dereference its saved virtqueue pointer after the virtqueue has been freed.
> 
> Vring access is not a prerequisite for tearing an endpoint down.  Remove
> the check so teardown always disables the backends and waits for in-flight
> commands before releasing their state.
> 
Can you describe the bug a little more?

If we have done vhost_scsi_set_endpoint successfully, are you saying a later
call to vhost_scsi_clear_endpoint could fail the vhost_vq_access_ok check
and not teardown the endpoint?

Or are you saying that even if we never did vhost_scsi_set_endpoint then
when vhost_scsi_release is called, a command could be running in the
vhost-scsi driver and so we need to do something? Is the something a
"vhost_vq_set_backend(vq, NULL)" and vhost_scsi_flush(vs)?