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

[sashiko-bot@kernel.org]

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