Re: [PATCH 2/4] vhost/vsock: add VHOST_RESET_OWNER ioctl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Concurrent host network traffic can trigger a Use-After-Free of `vhost_worker` during VHOST_RESET_OWNER.
- [High] Pending network work becomes permanently disabled if queued to a destroyed worker during VHOST_RESET_OWNER.
- [Medium] Useless O(N) system-wide socket iteration in `vhost_vsock_reset_owner` causing dead code and lock contention.
--

commit fe82b486d339416d80c28b16f13b7f0d1014a742
Author: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

vhost/vsock: add VHOST_RESET_OWNER ioctl

This ioctl is needed for QEMU's CPR (checkpoint-restore) migration of
the guest with vhost-vsock device.  For this to work, we need to reset
the device ownership on the source side by calling RESET_OWNER, and then
claim it on the dest side by calling SET_OWNER.  We expect not to lose any
AF_VSOCK connection while this happens.

> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index b12221ce6faf2..e629886e5cf84 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c

[ ... ]

> @@ -894,6 +894,32 @@ static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
>  	return -EFAULT;
>  }
>  
> +static int vhost_vsock_reset_owner(struct vhost_vsock *vsock)
> +{
> +	struct vhost_iotlb *umem;
> +	long err;
> +
> +	mutex_lock(&vsock->dev.mutex);
> +	err = vhost_dev_check_owner(&vsock->dev);
> +	if (err)
> +		goto done;
> +	umem = vhost_dev_reset_owner_prepare();
> +	if (!umem) {
> +		err = -ENOMEM;
> +		goto done;
> +	}
> +	/* Follows vhost_vsock_dev_release closely except for guest_cid drop */
> +	vsock_for_each_connected_socket(&vhost_transport.transport,
> +					vhost_vsock_reset_orphans);

[Severity: Medium]
Is this iteration necessary?

Unlike the release path, this ioctl deliberately keeps vsock->guest_cid
in vhost_vsock_hash to preserve connections across QEMU CPR.

Because the CID remains valid, when vhost_vsock_reset_orphans() iterates over
each system socket and calls:

  vhost_vsock_get(vsk->remote_addr.svm_cid, sock_net(sk))

it will successfully find the vsock and immediately return without performing
any cleanup.

Does this perform a useless system-wide traversal over all vsock connections
under the vsock_table_lock spinlock, unnecessarily wasting CPU cycles and
causing lock contention?

> +	vhost_vsock_drop_backends(vsock);
> +	vhost_vsock_flush(vsock);
> +	vhost_dev_stop(&vsock->dev);
> +	vhost_dev_reset_owner(&vsock->dev, umem);

[Severity: Critical]
Can concurrent host network traffic trigger a use-after-free of the
vhost_worker here?

Since vsock intentionally remains in vhost_vsock_hash to preserve connections
across CPR, a concurrent thread sending a packet can still resolve the vsock
object.

If the network thread reads vq->worker under the RCU read lock in
vhost_vq_work_queue() just before vhost_dev_reset_owner() calls
vhost_workers_free(), could we see a sequence like this:

Thread 1 (network):
vhost_transport_send_pkt()
  vhost_vq_work_queue()
    worker = rcu_dereference(vq->worker);

Thread 2 (ioctl):
vhost_vsock_reset_owner()
  vhost_dev_reset_owner()
    vhost_dev_cleanup()
      vhost_workers_free()
        kfree(worker);

Thread 1:
    vhost_worker_queue(worker, work); // use-after-free

Since vhost_workers_free() frees the worker without waiting for an RCU grace
period via synchronize_rcu(), does this leave a window for the network
thread to dereference the freed worker?

[Severity: High]
Does this sequence permanently disable pending network work if queued to a
destroyed worker?

If vhost_transport_send_pkt() races with vhost_vsock_reset_owner(), it calls
vhost_vq_work_queue() which sets the VHOST_WORK_QUEUED flag:

  if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
      ...
  }

If this happens right after vhost_vsock_flush() finishes, but before
vhost_workers_free() destroys the worker, the old worker thread is stopped
before it can process the list and clear the VHOST_WORK_QUEUED flag.

When the VM is resumed and a new worker is created, vhost_vsock_start() kicks
the worker by calling:

  vhost_vq_work_queue(&vsock->vqs[VSOCK_VQ_RX], &vsock->send_pkt_work);

Because the VHOST_WORK_QUEUED flag is still set to 1 from the destroyed worker,
test_and_set_bit() returns true. Will the work be skipped instead of being
added to the new worker's list, causing packets in send_pkt_queue to
permanently stall?

> +done:
> +	mutex_unlock(&vsock->dev.mutex);
> +	return err;
> +}

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