Re: [PATCH v5 4/5] vhost: synchronize with RCU readers when freeing workers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The placement of `synchronize_rcu()` and `vhost_dev_flush()` at the end of `vhost_dev_cleanup()` leaves destructive VQ operations unprotected against concurrently queued work, leading to data races and potential UAF/NULL-derefs.
- [Medium] vhost_workers_free() locklessly clearing vq->worker subverts vhost_worker_killed() synchronization, leading to leaked unexecuted work.
--

--- Patch [4]: [PATCH v5 4/5] vhost: synchronize with RCU readers when freeing workers ---
commit da9c9bade2016bfb46482de3d7f3d7b75b894fbc
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

vhost: synchronize with RCU readers when freeing workers

This commit adds synchronize_rcu() and vhost_dev_flush() when freeing vhost
workers. It intends to prevent a race condition where a lockless reader queues
work on a worker that is concurrently being freed.

> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 4c525b3e16ead..d6e235c25254c 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -729,6 +729,17 @@ static void vhost_workers_free(struct vhost_dev *dev)
>  
>  	for (i = 0; i < dev->nvqs; i++)
>  		rcu_assign_pointer(dev->vqs[i]->worker, NULL);

[Severity: Medium]
Does clearing vq->worker here subvert the synchronization in
vhost_worker_killed()?

If a fatal signal kills the worker thread, vhost_worker_killed() runs:

drivers/vhost/vhost.c:vhost_worker_killed() {
    ...
    if (worker == rcu_dereference_check(vq->worker, lockdep_is_held(&vq->mutex))) {
        rcu_assign_pointer(vq->worker, NULL);
        attach_cnt++;
    }
    ...
    if (attach_cnt)
        synchronize_rcu();
}

Because we've already cleared vq->worker locklessly above,
vhost_worker_killed() will see a NULL pointer, leave attach_cnt at 0, and skip
synchronize_rcu().

If a lockless reader then queues work, vhost_dev_flush() will silently
abort because worker->killed is true:

drivers/vhost/vhost.c:__vhost_worker_flush() {
    if (!worker->attachment_cnt || worker->killed)
        return;
}

Will this leave the queued work unexecuted and permanently break the
virtqueue by leaving VHOST_WORK_QUEUED set?

> +
> +	/*
> +	 * vhost_vq_work_queue() reads vq->worker under rcu_read_lock(), so a
> +	 * reader that fetched a worker before we cleared the pointers above
> +	 * may still be queueing work on it.  Wait for those readers to
> +	 * finish, then flush so any work they queued runs (clearing
> +	 * VHOST_WORK_QUEUED) before the workers are freed.
> +	 */
> +	synchronize_rcu();
> +	vhost_dev_flush(dev);

[Severity: Medium]
Does placing the flush here cause a use-after-free with the VQ state?

vhost_dev_cleanup() tears down VQ state locklessly before calling
vhost_workers_free():

drivers/vhost/vhost.c:vhost_dev_cleanup() {
    ...
    for (i = 0; i < dev->nvqs; ++i) {
        vhost_vq_reset(dev, dev->vqs[i]);
    }
    vhost_dev_free_iovecs(dev);
    ...
    vhost_workers_free(dev);
}

If a lockless sender (like the upcoming VHOST_RESET_OWNER support in vsock)
queues work that executes concurrently with vhost_dev_cleanup(), the
flushed work will run after the VQ state and memory maps are already
destroyed.

> +
>  	/*
>  	 * 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/20260720102241.371610-1-andrey.drobyshev@virtuozzo.com?part=4