[PATCH] vhost: clear vq->worker under vq->mutex when freeing workers

[PATCH] vhost: clear vq->worker under vq->mutex when freeing workers

[Andrey Drobyshev]

Every other update of vq->worker is done under vq->mutex - the worker
attach/swap ioctls and vhost_worker_killed().  vhost_workers_free() is
the sole exception: it clears vq->worker without holding the lock.

The effect is harmless in practice, as this only happens while the
owning process (and thus the whole device) is dying, but the lockless
write is inconsistent with the rest of the code.  Clear vq->worker under
vq->mutex, like everyone else, so that all writers of vq->worker follow
the same locking rule.

This issue was found by Sashiko AI review.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 drivers/vhost/vhost.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 4c525b3e16ea..dbb6cb5eccea 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -722,13 +722,19 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
 static void vhost_workers_free(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
+	struct vhost_virtqueue *vq;
 	unsigned long i;
 
 	if (!dev->use_worker)
 		return;
 
-	for (i = 0; i < dev->nvqs; i++)
-		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
+	for (i = 0; i < dev->nvqs; i++) {
+		vq = dev->vqs[i];
+
+		mutex_lock(&vq->mutex);
+		rcu_assign_pointer(vq->worker, NULL);
+		mutex_unlock(&vq->mutex);
+	}
 	/*
 	 * Free the default worker we created and cleanup workers userspace
 	 * created but couldn't clean up (it forgot or crashed).
-- 
2.47.1

Re: [PATCH] vhost: clear vq->worker under vq->mutex when freeing workers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch is an incomplete fix that introduces redundant locking while failing to achieve its stated goal.
--

--- Patch [1]: [PATCH] vhost: clear vq->worker under vq->mutex when freeing workers ---
commit b3a889e45c548a1e7c25fa886c0a431a661a7155
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

vhost: clear vq->worker under vq->mutex when freeing workers

The commit adds mutex locking around the clearing of vq->worker in
vhost_workers_free(). The commit message states this is to eliminate the
sole instance where vq->worker is written locklessly.

> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 4c525b3e16ead..dbb6cb5eccea1 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -722,13 +722,19 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
>  static void vhost_workers_free(struct vhost_dev *dev)
>  {
>  	struct vhost_worker *worker;
> +	struct vhost_virtqueue *vq;
>  	unsigned long i;
>  
>  	if (!dev->use_worker)
>  		return;
>  
> -	for (i = 0; i < dev->nvqs; i++)
> -		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
> +	for (i = 0; i < dev->nvqs; i++) {
> +		vq = dev->vqs[i];
> +
> +		mutex_lock(&vq->mutex);
> +		rcu_assign_pointer(vq->worker, NULL);
> +		mutex_unlock(&vq->mutex);
> +	}

[Severity: Medium]
The commit message notes that vhost_workers_free() is the sole exception
where vq->worker is cleared locklessly, but doesn't vhost_vq_reset() also
do this?

During device teardown, vhost_dev_cleanup() is called, which resets the
virtqueues before freeing the workers:

vhost_dev_cleanup() {
    ...
    for (i = 0; i < dev->nvqs; ++i) {
        ...
        vhost_vq_reset(dev, dev->vqs[i]);
            // Inside vhost_vq_reset():
            rcu_assign_pointer(vq->worker, NULL);
    }
    ...
    vhost_workers_free(dev);
}

Because vhost_vq_reset() already unconditionally clears vq->worker
locklessly, won't this new locking block in vhost_workers_free() simply
acquire the lock to write NULL to a pointer that is already NULL?

Since the lockless write still exists in vhost_vq_reset(), does this patch
fully achieve its stated goal, or does it add an unnecessary lock cycle?

>  	/*
>  	 * Free the default worker we created and cleanup workers userspace
>  	 * created but couldn't clean up (it forgot or crashed).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723153310.745855-1-andrey.drobyshev@virtuozzo.com?part=1