[PATCH v4 0/5] vhost/vsock: add support for VHOST_RESET_OWNER and CPR migration

[PATCH v4 0/5] vhost/vsock: add support for VHOST_RESET_OWNER and CPR migration

[Andrey Drobyshev]

The main change since v3: as suggested by Stefano, the worker teardown
race is now fixed where it actually lives, i.e. in vhost_workers_free(),
instead of being guarded from the vsock side.  The new patch 4 makes the
teardown wait out the RCU readers which might still be queueing work on
a worker, and then run whatever they queued, before the workers are
freed.

With the teardown itself made safe, queueing work on a stopped device
is harmless again: the work handlers check the backend under vq->mutex
and simply return.  So the backend guards in send_pkt()/cancel_pkt()
and the synchronize_rcu() in reset_owner() from v3 are no longer needed
and are gone.

v3 -> v4:

  * Patch 2:
    - rename 'started' -> 'ever_started';
    - reword commit message;
  * Patch 3: reword commit message and code comment;
  * Patch 4 (NEW) ("vhost: synchronize with RCU readers when freeing workers"):
    - fix the vq->worker UAF and the stuck VHOST_WORK_QUEUED bit
      generically in vhost_workers_free();
  * Patch 4 -> 5 ("vhost/vsock: add VHOST_RESET_OWNER ioctl"):
    - drop the backend guards in send_pkt()/cancel_pkt() and the
      synchronize_rcu() in reset_owner() - covered by patch 4 now;
    - make vhost_vsock_reset_owner() return long;
    - reword commit message.

v3: https://lore.kernel.org/virtualization/20260625155416.480669-1-andrey.drobyshev@virtuozzo.com

Andrey Drobyshev (3):
  vhost/vsock: suppress EHOSTUNREACH fast-fail during CPR pause
  vhost/vsock: re-scan TX virtqueue on device start
  vhost: synchronize with RCU readers when freeing workers

Pavel Tikhomirov (2):
  vhost/vsock: split out vhost_vsock_drop_backends helper
  vhost/vsock: add VHOST_RESET_OWNER ioctl

 drivers/vhost/vhost.c | 15 ++++++++++
 drivers/vhost/vsock.c | 80 +++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 76 insertions(+), 19 deletions(-)

-- 
2.47.1

[PATCH v4 1/5] vhost/vsock: split out vhost_vsock_drop_backends helper

[Andrey Drobyshev]

From: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

Split the actual backend dropping part from vhost_vsock_stop.  We're
going to need it for the VHOST_RESET_OWNER implementation in the
following patch, when vsock->dev.mutex is already taken and owner is
checked.

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vhost/vsock.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index 9aaab6bb8061..b12221ce6faf 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -664,9 +664,24 @@ static int vhost_vsock_start(struct vhost_vsock *vsock)
 	return ret;
 }
 
-static int vhost_vsock_stop(struct vhost_vsock *vsock, bool check_owner)
+static void vhost_vsock_drop_backends(struct vhost_vsock *vsock)
 {
+	struct vhost_virtqueue *vq;
 	size_t i;
+
+	lockdep_assert_held(&vsock->dev.mutex);
+
+	for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
+		vq = &vsock->vqs[i];
+
+		mutex_lock(&vq->mutex);
+		vhost_vq_set_backend(vq, NULL);
+		mutex_unlock(&vq->mutex);
+	}
+}
+
+static int vhost_vsock_stop(struct vhost_vsock *vsock, bool check_owner)
+{
 	int ret = 0;
 
 	mutex_lock(&vsock->dev.mutex);
@@ -677,14 +692,7 @@ static int vhost_vsock_stop(struct vhost_vsock *vsock, bool check_owner)
 			goto err;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
-		struct vhost_virtqueue *vq = &vsock->vqs[i];
-
-		mutex_lock(&vq->mutex);
-		vhost_vq_set_backend(vq, NULL);
-		mutex_unlock(&vq->mutex);
-	}
-
+	vhost_vsock_drop_backends(vsock);
 err:
 	mutex_unlock(&vsock->dev.mutex);
 	return ret;
-- 
2.47.1

[PATCH v4 2/5] vhost/vsock: suppress EHOSTUNREACH fast-fail during CPR pause

[Andrey Drobyshev]

Earlier commit bb26ed5f3a8b ("vhost/vsock: Refuse the connection
immediately when guest isn't ready") added a fast-fail in
vhost_transport_send_pkt().  It rejects every host send with -EHOSTUNREACH
until the destination calls SET_RUNNING(1).  The fast-fail condition checks
whether device's backends are dropped, and if they're, the guest is
considered to be not ready.

However, there might be other reasons for backends to be nulled.  In
particular, when QEMU is performing CPR (checkpoint-restore) migration,
device ownership is being RESET and SET again, which leads to backends
drop and reattach.  If we end up connecting during this window, an
AF_VSOCK client gets -EHOSTUNREACH, which is wrong.

Add an 'ever_started' flag which is set once in vhost_vsock_start() and is
never cleared.  The behaviour changes to:

  * When device was never started -> flag is unset -> no listener can
    exist yet -> fast-fail;
  * Once the device starts -> flag is set -> we don't fast-fail ->
    we queue and preserve during any later stop / CPR pause.

The VHOST_RESET_OWNER ioctl is implemented in a following patch, and
without RESET_OWNER the problem we fix here isn't manifesting - thus
this patch is a preparation to support RESET_OWNER.

Important caveat: after the first start, a connect during any stopped
window is queued instead of fast-failed.  That was the behaviour before
the patch bb26ed5f3a8b, and we're restoring it now.  However we still
keep the behaviour originally intended by that commit (i.e. fast-fail if
there's no real listener yet) while fixing the CPR path.

Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 drivers/vhost/vsock.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index b12221ce6faf..27169a09e87e 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -61,6 +61,7 @@ struct vhost_vsock {
 
 	u32 guest_cid;
 	bool seqpacket_allow;
+	bool ever_started; /* set on first SET_RUNNING(1); never cleared */
 };
 
 static u32 vhost_transport_get_local_cid(void)
@@ -302,17 +303,12 @@ vhost_transport_send_pkt(struct sk_buff *skb, struct net *net)
 		return -ENODEV;
 	}
 
-	/* Fast-fail if the guest hasn't enabled the RX vq yet. Queuing the packet
-	 * and making the caller wait is pointless: even if the guest manages to init
-	 * within the timeout, it'll immediately reply with RST, because there's no
-	 * listener on the port yet.
-	 *
-	 * vhost_vq_get_backend() without vq->mutex is acceptable here: locking
-	 * the mutex would be too expensive in this hot path, and we already have
-	 * all the outcomes covered: if the backend becomes NULL right after the check,
-	 * vhost_transport_do_send_pkt() will check it under the mutex anyway.
+	/* Fast-fail until the guest first enables the device (SET_RUNNING(1)).
+	 * Before that there is no listener, so queuing is pointless.
+	 * 'ever_started' is never cleared, so once we're up we keep queuing
+	 * across later stop / CPR-pause windows.
 	 */
-	if (unlikely(!data_race(vhost_vq_get_backend(&vsock->vqs[VSOCK_VQ_RX])))) {
+	if (unlikely(!READ_ONCE(vsock->ever_started))) {
 		rcu_read_unlock();
 		kfree_skb(skb);
 		return -EHOSTUNREACH;
@@ -640,6 +636,11 @@ static int vhost_vsock_start(struct vhost_vsock *vsock)
 		mutex_unlock(&vq->mutex);
 	}
 
+	/* Set 'ever_started' flag on the first start; never cleared, so send_pkt
+	 * keeps queuing (instead of fast-failing) on later stop / CPR pauses.
+	 */
+	WRITE_ONCE(vsock->ever_started, true);
+
 	/* Some packets may have been queued before the device was started,
 	 * let's kick the send worker to send them.
 	 */
@@ -728,6 +729,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
 
 	vsock->guest_cid = 0; /* no CID assigned yet */
 	vsock->seqpacket_allow = false;
+	vsock->ever_started = false;
 
 	atomic_set(&vsock->queued_replies, 0);
 
-- 
2.47.1

[PATCH v4 3/5] vhost/vsock: re-scan TX virtqueue on device start

[Andrey Drobyshev]

During QEMU CPR live-update (and VHOST_RESET_OWNER in general) the guest
keeps running while the host drops and later re-attaches vhost backends.
If the guest adds a buffer to the TX virtqueue (guest->host) and kicks
while the backend is temporarily NULL (between vhost_vsock_drop_backends()
and the next vhost_vsock_start()), then the kick is delivered to the
vhost worker, handle_tx_kick() sees a NULL backend and returns, and the
kick signal is consumed.  The buffer is then left in the ring.

Then upon device start vhost_vsock_start() only re-kicks the RX send
worker, never the TX VQ, so the buffer is processed only if the guest
happens to kick again.  But if the guest itself is now waiting for data
from the host, it will never kick TX VQ again, and we end up in a
deadlock.

The issue itself is pre-existing, but it only manifests during a device
pause caused by VHOST_RESET_OWNER.  Namely, the deadlock is reproduced
during active host->guest socat data transfer under multiple consecutive
CPR live-update's.

To fix this, in vhost_vsock_start(), after kicking the RX send worker, also
queue the TX vq poll so any buffers the guest enqueued while we were paused
get scanned.

The VHOST_RESET_OWNER ioctl itself is implemented in the following
patch, thus this patch is a preparation to support VHOST_RESET_OWNER.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 drivers/vhost/vsock.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index 27169a09e87e..d5022d21120b 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -646,6 +646,13 @@ static int vhost_vsock_start(struct vhost_vsock *vsock)
 	 */
 	vhost_vq_work_queue(&vsock->vqs[VSOCK_VQ_RX], &vsock->send_pkt_work);
 
+	/* The guest may have added TX buffers while the device was stopped
+	 * (e.g. across VHOST_RESET_OWNER) and their kicks got consumed by
+	 * the NULL-backend window.  Re-scan the TX VQ, mirroring the RX
+	 * send-worker kick above.
+	 */
+	vhost_poll_queue(&vsock->vqs[VSOCK_VQ_TX].poll);
+
 	mutex_unlock(&vsock->dev.mutex);
 	return 0;
 
-- 
2.47.1

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

[Andrey Drobyshev]

vhost_vq_work_queue() only holds the RCU read lock while it dereferences
vq->worker and queues work on it.  vhost_workers_free() however clears
the vq->worker pointers and immediately frees the workers, without
waiting for a grace period.  A caller that fetched the worker right
before the pointer was cleared can therefore still be queueing work on
it while it is freed.  And even when the queueing itself wins the race,
the work is never run, so its VHOST_WORK_QUEUED bit stays set and all
future attempts to queue it are silently skipped.

None of the current callers can actually hit this: net and scsi stop
their virtqueues before the workers are freed, and vsock unhashes the
device and does synchronize_rcu() of its own in vhost_vsock_dev_release()
before the workers go away.  But the upcoming VHOST_RESET_OWNER support
in vhost-vsock keeps the device hashed while its workers are freed, so
the lockless send/cancel paths become able to race with the teardown.

Close this the way vhost_worker_killed() already does: clear the
vq->worker pointers, wait for a grace period, run whatever the last
readers may have queued, and only then free the workers.  The
synchronize_rcu() is skipped if the device has no workers, so cleanup of
devices which never got an owner stays cheap.

Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 drivers/vhost/vhost.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 4c525b3e16ea..0d1414d40f4e 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -729,6 +729,21 @@ static void vhost_workers_free(struct vhost_dev *dev)
 
 	for (i = 0; i < dev->nvqs; i++)
 		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
+
+	/*
+	 * vhost_vq_work_queue() reads vq->worker under rcu_read_lock(), so a
+	 * caller that fetched a worker before we cleared the pointers above
+	 * may still be about to queue work on it.  Wait for those RCU readers
+	 * to finish before freeing the worker, then run whatever they queued
+	 * so nothing is left with VHOST_WORK_QUEUED set.  Mirrors
+	 * vhost_worker_killed().
+	 */
+	if (!xa_empty(&dev->worker_xa)) {
+		synchronize_rcu();
+		xa_for_each(&dev->worker_xa, i, worker)
+			vhost_run_work_list(worker);
+	}
+
 	/*
 	 * 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 v4 4/5] vhost: synchronize with RCU readers when freeing workers

[Stefano Garzarella]

On Tue, Jul 14, 2026 at 06:16:37PM +0300, Andrey Drobyshev wrote:
>vhost_vq_work_queue() only holds the RCU read lock while it dereferences
>vq->worker and queues work on it.  vhost_workers_free() however clears
>the vq->worker pointers and immediately frees the workers, without
>waiting for a grace period.  A caller that fetched the worker right
>before the pointer was cleared can therefore still be queueing work on
>it while it is freed.  And even when the queueing itself wins the race,
>the work is never run, so its VHOST_WORK_QUEUED bit stays set and all
>future attempts to queue it are silently skipped.
>
>None of the current callers can actually hit this: net and scsi stop
>their virtqueues before the workers are freed, and vsock unhashes the
>device and does synchronize_rcu() of its own in vhost_vsock_dev_release()
>before the workers go away.  But the upcoming VHOST_RESET_OWNER support
>in vhost-vsock keeps the device hashed while its workers are freed, so
>the lockless send/cancel paths become able to race with the teardown.
>
>Close this the way vhost_worker_killed() already does: clear the
>vq->worker pointers, wait for a grace period, run whatever the last
>readers may have queued, and only then free the workers.  The
>synchronize_rcu() is skipped if the device has no workers, so cleanup of
>devices which never got an owner stays cheap.
>

Do we need a Fixes tag for this?

Thanks for pointing out that the issue wasn't occurring, but I think we 
should add it because it's a sneaky problem we discovered by chance.
IMO the code should already have `synchronize_rcu()` after 
`rcu_assign_pointer()` loop.

@Michael, what do you think?

>Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>---
> drivers/vhost/vhost.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 4c525b3e16ea..0d1414d40f4e 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -729,6 +729,21 @@ static void vhost_workers_free(struct vhost_dev *dev)
>
> 	for (i = 0; i < dev->nvqs; i++)
> 		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
>+
>+	/*
>+	 * vhost_vq_work_queue() reads vq->worker under rcu_read_lock(), so a
>+	 * caller that fetched a worker before we cleared the pointers above
>+	 * may still be about to queue work on it.  Wait for those RCU readers
>+	 * to finish before freeing the worker, then run whatever they queued
>+	 * so nothing is left with VHOST_WORK_QUEUED set.  Mirrors
>+	 * vhost_worker_killed().
>+	 */
>+	if (!xa_empty(&dev->worker_xa)) {
>+		synchronize_rcu();
>+		xa_for_each(&dev->worker_xa, i, worker)
>+			vhost_run_work_list(worker);
>+	}
>+

Following sashiko review [1], I tried to undersand why we need this, but 
TBH I'm really confused. That said, this seems wrong also because it 
will work only with vhost_tasks, and not with kthreads.

IIUC vhost_worker_killed() will be called anyway when calling 
vhost_worker_destroy(). For vhost_tasks, it will call 
vhost_task_do_stop() that calls vhost_task_stop(). This sets 
VHOST_TASK_FLAGS_STOP and wait the worker on vtsk->exited before freeing 
stuff. The worker breaks the loop and calls vtsk->handle_sigkill() that 
is exactly vhost_worker_killed() you mentioned we are mirroring here.

So, why we need this?

Should be enough to call synchronize_rcu() in any case after the 
rcu_assign_pointer() loop?

Thanks,
Stefano

[1] 
https://sashiko.dev/#/patchset/20260714151638.143019-1-andrey.drobyshev@virtuozzo.com?part=4

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

[Andrey Drobyshev]

On 7/16/26 11:57 AM, Stefano Garzarella wrote:
> On Tue, Jul 14, 2026 at 06:16:37PM +0300, Andrey Drobyshev wrote:
>> vhost_vq_work_queue() only holds the RCU read lock while it dereferences
>> vq->worker and queues work on it.  vhost_workers_free() however clears
>> the vq->worker pointers and immediately frees the workers, without
>> waiting for a grace period.  A caller that fetched the worker right
>> before the pointer was cleared can therefore still be queueing work on
>> it while it is freed.  And even when the queueing itself wins the race,
>> the work is never run, so its VHOST_WORK_QUEUED bit stays set and all
>> future attempts to queue it are silently skipped.
>>
>> None of the current callers can actually hit this: net and scsi stop
>> their virtqueues before the workers are freed, and vsock unhashes the
>> device and does synchronize_rcu() of its own in vhost_vsock_dev_release()
>> before the workers go away.  But the upcoming VHOST_RESET_OWNER support
>> in vhost-vsock keeps the device hashed while its workers are freed, so
>> the lockless send/cancel paths become able to race with the teardown.
>>
>> Close this the way vhost_worker_killed() already does: clear the
>> vq->worker pointers, wait for a grace period, run whatever the last
>> readers may have queued, and only then free the workers.  The
>> synchronize_rcu() is skipped if the device has no workers, so cleanup of
>> devices which never got an owner stays cheap.
>>
> 
> Do we need a Fixes tag for this?
>

I'm guessing it should be:

Fixes: 228a27cf78af ("vhost: Allow worker switching while work is queueing")

> Thanks for pointing out that the issue wasn't occurring, but I think we 
> should add it because it's a sneaky problem we discovered by chance.
> IMO the code should already have `synchronize_rcu()` after 
> `rcu_assign_pointer()` loop.
> 
> @Michael, what do you think?
> 
>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>> drivers/vhost/vhost.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index 4c525b3e16ea..0d1414d40f4e 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -729,6 +729,21 @@ static void vhost_workers_free(struct vhost_dev *dev)
>>
>> 	for (i = 0; i < dev->nvqs; i++)
>> 		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
>> +
>> +	/*
>> +	 * vhost_vq_work_queue() reads vq->worker under rcu_read_lock(), so a
>> +	 * caller that fetched a worker before we cleared the pointers above
>> +	 * may still be about to queue work on it.  Wait for those RCU readers
>> +	 * to finish before freeing the worker, then run whatever they queued
>> +	 * so nothing is left with VHOST_WORK_QUEUED set.  Mirrors
>> +	 * vhost_worker_killed().
>> +	 */
>> +	if (!xa_empty(&dev->worker_xa)) {
>> +		synchronize_rcu();
>> +		xa_for_each(&dev->worker_xa, i, worker)
>> +			vhost_run_work_list(worker);
>> +	}
>> +
> 
> Following sashiko review [1], I tried to undersand why we need this, but 
> TBH I'm really confused. That said, this seems wrong also because it 
> will work only with vhost_tasks, and not with kthreads.
> 
> IIUC vhost_worker_killed() will be called anyway when calling 
> vhost_worker_destroy(). For vhost_tasks, it will call 
> vhost_task_do_stop() that calls vhost_task_stop(). This sets 
> VHOST_TASK_FLAGS_STOP and wait the worker on vtsk->exited before freeing 
> stuff. The worker breaks the loop and calls vtsk->handle_sigkill() that 
> is exactly vhost_worker_killed() you mentioned we are mirroring here.
>

Hmm, are we sure it's the case for our codepath?  Looking at the
vhost_task loop function:

> static int vhost_task_fn(void *data)                                            
> {
>     for (;;) {                                                                                                                                                   
>         if (signal_pending(current)) {
>             if (get_signal(&ksig))                                                 
>                 break;                                                          
>         }                                                                       
>         ...
>         if (test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
>             __set_current_state(TASK_RUNNING);
>             break;
>         }                                                                          
>         did_work = vtsk->fn(vtsk->data);                                           
>         ...
>     }
>                                                                                 
>     ...
>
>     if (!test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {                       
>         set_bit(VHOST_TASK_FLAGS_KILLED, &vtsk->flags);                         
>         vtsk->handle_sigkill(vtsk->data);                                       
>     }                                                                           
>     ...                                                                
> } 

AFAICT, we exit the loop in 2 cases: signal delivery or STOP bit
setting.  Like you said, STOP is set by vhost_task_stop.  E.g. for our
RESET_OWNER case:

vhost_vsock_reset_owner()
  vhost_dev_reset_owner()
    vhost_dev_cleanup()
      vhost_workers_free()
        vhost_worker_destroy()
          vhost_task_stop()  // for vhost_task_ops backend
            set_bit(VHOST_TASK_FLAGS_STOP)

So, first of all, actual work by .fn() callback is done after the exit
checks, therefore we skip it - no chance to drain there.

Secondly, the handle_sigkill() callback is deliberately NOT called in
the STOP case and only called on fatal signal delivery.  And for
vhost_task backend the .handle_sigkill() callback is exactly
vhost_worker_killed().

So my understanding is: if we only call synchronize_rcu() here and leave
this path undrained, then whatever work which was put by send_pkt() for
the worker currently being freed - will be lost.  Please correct me if
I'm wrong.

That said, I agree that vhost_run_work_list() will only work with
vhost_task backend, not with kthreads backend.  If we do
vhost_worker_flush() instead - I guess it'll keep the drain here, yet
become backend-agnostic. I.e.:

> +       if (!xa_empty(&dev->worker_xa)) {
> +               synchronize_rcu();
> +               xa_for_each(&dev->worker_xa, i, worker)
> +                       vhost_worker_flush(worker);
> +       }

With the last 2 lines being equivalent to just calling
vhost_dev_flush(dev).  And once we become backend-agnostic here, I'm
guessing the warning reported by Sashiko should be dealt with as well.

WDYT?

Andrey


> So, why we need this?
> 
> Should be enough to call synchronize_rcu() in any case after the 
> rcu_assign_pointer() loop?
> 
> Thanks,
> Stefano
>
> [1] 
> https://sashiko.dev/#/patchset/20260714151638.143019-1-andrey.drobyshev@virtuozzo.com?part=4
> 

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

[Stefano Garzarella]

On Thu, Jul 16, 2026 at 06:39:48PM +0300, Andrey Drobyshev wrote:
>On 7/16/26 11:57 AM, Stefano Garzarella wrote:
>> On Tue, Jul 14, 2026 at 06:16:37PM +0300, Andrey Drobyshev wrote:
>>> vhost_vq_work_queue() only holds the RCU read lock while it dereferences
>>> vq->worker and queues work on it.  vhost_workers_free() however clears
>>> the vq->worker pointers and immediately frees the workers, without
>>> waiting for a grace period.  A caller that fetched the worker right
>>> before the pointer was cleared can therefore still be queueing work on
>>> it while it is freed.  And even when the queueing itself wins the race,
>>> the work is never run, so its VHOST_WORK_QUEUED bit stays set and all
>>> future attempts to queue it are silently skipped.
>>>
>>> None of the current callers can actually hit this: net and scsi stop
>>> their virtqueues before the workers are freed, and vsock unhashes the
>>> device and does synchronize_rcu() of its own in vhost_vsock_dev_release()
>>> before the workers go away.  But the upcoming VHOST_RESET_OWNER support
>>> in vhost-vsock keeps the device hashed while its workers are freed, so
>>> the lockless send/cancel paths become able to race with the teardown.
>>>
>>> Close this the way vhost_worker_killed() already does: clear the
>>> vq->worker pointers, wait for a grace period, run whatever the last
>>> readers may have queued, and only then free the workers.  The
>>> synchronize_rcu() is skipped if the device has no workers, so cleanup of
>>> devices which never got an owner stays cheap.
>>>
>>
>> Do we need a Fixes tag for this?
>>
>
>I'm guessing it should be:
>
>Fixes: 228a27cf78af ("vhost: Allow worker switching while work is queueing")
>
>> Thanks for pointing out that the issue wasn't occurring, but I think we
>> should add it because it's a sneaky problem we discovered by chance.
>> IMO the code should already have `synchronize_rcu()` after
>> `rcu_assign_pointer()` loop.
>>
>> @Michael, what do you think?
>>
>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>>> ---
>>> drivers/vhost/vhost.c | 15 +++++++++++++++
>>> 1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>>> index 4c525b3e16ea..0d1414d40f4e 100644
>>> --- a/drivers/vhost/vhost.c
>>> +++ b/drivers/vhost/vhost.c
>>> @@ -729,6 +729,21 @@ static void vhost_workers_free(struct vhost_dev *dev)
>>>
>>> 	for (i = 0; i < dev->nvqs; i++)
>>> 		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
>>> +
>>> +	/*
>>> +	 * vhost_vq_work_queue() reads vq->worker under rcu_read_lock(), so a
>>> +	 * caller that fetched a worker before we cleared the pointers above
>>> +	 * may still be about to queue work on it.  Wait for those RCU readers
>>> +	 * to finish before freeing the worker, then run whatever they queued
>>> +	 * so nothing is left with VHOST_WORK_QUEUED set.  Mirrors
>>> +	 * vhost_worker_killed().
>>> +	 */
>>> +	if (!xa_empty(&dev->worker_xa)) {
>>> +		synchronize_rcu();
>>> +		xa_for_each(&dev->worker_xa, i, worker)
>>> +			vhost_run_work_list(worker);
>>> +	}
>>> +
>>
>> Following sashiko review [1], I tried to undersand why we need this, but
>> TBH I'm really confused. That said, this seems wrong also because it
>> will work only with vhost_tasks, and not with kthreads.
>>
>> IIUC vhost_worker_killed() will be called anyway when calling
>> vhost_worker_destroy(). For vhost_tasks, it will call
>> vhost_task_do_stop() that calls vhost_task_stop(). This sets
>> VHOST_TASK_FLAGS_STOP and wait the worker on vtsk->exited before freeing
>> stuff. The worker breaks the loop and calls vtsk->handle_sigkill() that
>> is exactly vhost_worker_killed() you mentioned we are mirroring here.
>>
>
>Hmm, are we sure it's the case for our codepath?  Looking at the
>vhost_task loop function:
>
>> static int vhost_task_fn(void *data)
>> {
>>     for (;;) {
>>         if (signal_pending(current)) {
>>             if (get_signal(&ksig))
>>                 break;
>>         }
>>         ...
>>         if (test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
>>             __set_current_state(TASK_RUNNING);
>>             break;
>>         }
>>         did_work = vtsk->fn(vtsk->data);
>>         ...
>>     }
>>
>>     ...
>>
>>     if (!test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
>>         set_bit(VHOST_TASK_FLAGS_KILLED, &vtsk->flags);
>>         vtsk->handle_sigkill(vtsk->data);
>>     }
>>     ...
>> }
>
>AFAICT, we exit the loop in 2 cases: signal delivery or STOP bit
>setting.  Like you said, STOP is set by vhost_task_stop.  E.g. for our
>RESET_OWNER case:
>
>vhost_vsock_reset_owner()
>  vhost_dev_reset_owner()
>    vhost_dev_cleanup()
>      vhost_workers_free()
>        vhost_worker_destroy()
>          vhost_task_stop()  // for vhost_task_ops backend
>            set_bit(VHOST_TASK_FLAGS_STOP)
>
>So, first of all, actual work by .fn() callback is done after the exit
>checks, therefore we skip it - no chance to drain there.
>
>Secondly, the handle_sigkill() callback is deliberately NOT called in
>the STOP case and only called on fatal signal delivery.  And for
>vhost_task backend the .handle_sigkill() callback is exactly
>vhost_worker_killed().
>
>So my understanding is: if we only call synchronize_rcu() here and leave
>this path undrained, then whatever work which was put by send_pkt() for
>the worker currently being freed - will be lost.  Please correct me if
>I'm wrong.

Yep, your right. But what will be the issue of loosing them?

IIUC we are not loosing any data, just avoiding some works that will be 
handled later when/if will set a new owner.

>
>That said, I agree that vhost_run_work_list() will only work with
>vhost_task backend, not with kthreads backend.  If we do
>vhost_worker_flush() instead - I guess it'll keep the drain here, yet
>become backend-agnostic. I.e.:
>
>> +       if (!xa_empty(&dev->worker_xa)) {
>> +               synchronize_rcu();
>> +               xa_for_each(&dev->worker_xa, i, worker)
>> +                       vhost_worker_flush(worker);
>> +       }
>
>With the last 2 lines being equivalent to just calling
>vhost_dev_flush(dev).  And once we become backend-agnostic here, I'm
>guessing the warning reported by Sashiko should be dealt with as well.

I'd avoid `if !xa_empty(&dev->worker_xa)` at all, and call 
synchronize_rcu() in any case.

About vhost_dev_flush(), we are calling it in several places, and maybe 
we should re-check them. E.g. we call in vhost_vsock_flush(), but it's 
also called by vhost_dev_stop(), maybe we can avoid to call 
vhost_vsock_flush() if we call vhost_dev_stop().

I'm not sure we really need another one here, but if you think some 
other works can be queued between the vhost_dev_stop() and the 
synchronize_rcu() we are adding here, then okay, it may have sense.

Thanks,
Stefano

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

[Andrey Drobyshev]

On 7/16/26 7:13 PM, Stefano Garzarella wrote:
> On Thu, Jul 16, 2026 at 06:39:48PM +0300, Andrey Drobyshev wrote:
>> On 7/16/26 11:57 AM, Stefano Garzarella wrote:
>>> On Tue, Jul 14, 2026 at 06:16:37PM +0300, Andrey Drobyshev wrote:
>>>> vhost_vq_work_queue() only holds the RCU read lock while it dereferences
>>>> vq->worker and queues work on it.  vhost_workers_free() however clears
>>>> the vq->worker pointers and immediately frees the workers, without
>>>> waiting for a grace period.  A caller that fetched the worker right
>>>> before the pointer was cleared can therefore still be queueing work on
>>>> it while it is freed.  And even when the queueing itself wins the race,
>>>> the work is never run, so its VHOST_WORK_QUEUED bit stays set and all
>>>> future attempts to queue it are silently skipped.
>>>>
>>>> None of the current callers can actually hit this: net and scsi stop
>>>> their virtqueues before the workers are freed, and vsock unhashes the
>>>> device and does synchronize_rcu() of its own in vhost_vsock_dev_release()
>>>> before the workers go away.  But the upcoming VHOST_RESET_OWNER support
>>>> in vhost-vsock keeps the device hashed while its workers are freed, so
>>>> the lockless send/cancel paths become able to race with the teardown.
>>>>
>>>> Close this the way vhost_worker_killed() already does: clear the
>>>> vq->worker pointers, wait for a grace period, run whatever the last
>>>> readers may have queued, and only then free the workers.  The
>>>> synchronize_rcu() is skipped if the device has no workers, so cleanup of
>>>> devices which never got an owner stays cheap.
>>>>
>>>
>>> Do we need a Fixes tag for this?
>>>
>>
>> I'm guessing it should be:
>>
>> Fixes: 228a27cf78af ("vhost: Allow worker switching while work is queueing")
>>
>>> Thanks for pointing out that the issue wasn't occurring, but I think we
>>> should add it because it's a sneaky problem we discovered by chance.
>>> IMO the code should already have `synchronize_rcu()` after
>>> `rcu_assign_pointer()` loop.
>>>
>>> @Michael, what do you think?
>>>
>>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>>>> ---
>>>> drivers/vhost/vhost.c | 15 +++++++++++++++
>>>> 1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>>>> index 4c525b3e16ea..0d1414d40f4e 100644
>>>> --- a/drivers/vhost/vhost.c
>>>> +++ b/drivers/vhost/vhost.c
>>>> @@ -729,6 +729,21 @@ static void vhost_workers_free(struct vhost_dev *dev)
>>>>
>>>> 	for (i = 0; i < dev->nvqs; i++)
>>>> 		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
>>>> +
>>>> +	/*
>>>> +	 * vhost_vq_work_queue() reads vq->worker under rcu_read_lock(), so a
>>>> +	 * caller that fetched a worker before we cleared the pointers above
>>>> +	 * may still be about to queue work on it.  Wait for those RCU readers
>>>> +	 * to finish before freeing the worker, then run whatever they queued
>>>> +	 * so nothing is left with VHOST_WORK_QUEUED set.  Mirrors
>>>> +	 * vhost_worker_killed().
>>>> +	 */
>>>> +	if (!xa_empty(&dev->worker_xa)) {
>>>> +		synchronize_rcu();
>>>> +		xa_for_each(&dev->worker_xa, i, worker)
>>>> +			vhost_run_work_list(worker);
>>>> +	}
>>>> +
>>>
>>> Following sashiko review [1], I tried to undersand why we need this, but
>>> TBH I'm really confused. That said, this seems wrong also because it
>>> will work only with vhost_tasks, and not with kthreads.
>>>
>>> IIUC vhost_worker_killed() will be called anyway when calling
>>> vhost_worker_destroy(). For vhost_tasks, it will call
>>> vhost_task_do_stop() that calls vhost_task_stop(). This sets
>>> VHOST_TASK_FLAGS_STOP and wait the worker on vtsk->exited before freeing
>>> stuff. The worker breaks the loop and calls vtsk->handle_sigkill() that
>>> is exactly vhost_worker_killed() you mentioned we are mirroring here.
>>>
>>
>> Hmm, are we sure it's the case for our codepath?  Looking at the
>> vhost_task loop function:
>>
>>> static int vhost_task_fn(void *data)
>>> {
>>>     for (;;) {
>>>         if (signal_pending(current)) {
>>>             if (get_signal(&ksig))
>>>                 break;
>>>         }
>>>         ...
>>>         if (test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
>>>             __set_current_state(TASK_RUNNING);
>>>             break;
>>>         }
>>>         did_work = vtsk->fn(vtsk->data);
>>>         ...
>>>     }
>>>
>>>     ...
>>>
>>>     if (!test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
>>>         set_bit(VHOST_TASK_FLAGS_KILLED, &vtsk->flags);
>>>         vtsk->handle_sigkill(vtsk->data);
>>>     }
>>>     ...
>>> }
>>
>> AFAICT, we exit the loop in 2 cases: signal delivery or STOP bit
>> setting.  Like you said, STOP is set by vhost_task_stop.  E.g. for our
>> RESET_OWNER case:
>>
>> vhost_vsock_reset_owner()
>>  vhost_dev_reset_owner()
>>    vhost_dev_cleanup()
>>      vhost_workers_free()
>>        vhost_worker_destroy()
>>          vhost_task_stop()  // for vhost_task_ops backend
>>            set_bit(VHOST_TASK_FLAGS_STOP)
>>
>> So, first of all, actual work by .fn() callback is done after the exit
>> checks, therefore we skip it - no chance to drain there.
>>
>> Secondly, the handle_sigkill() callback is deliberately NOT called in
>> the STOP case and only called on fatal signal delivery.  And for
>> vhost_task backend the .handle_sigkill() callback is exactly
>> vhost_worker_killed().
>>
>> So my understanding is: if we only call synchronize_rcu() here and leave
>> this path undrained, then whatever work which was put by send_pkt() for
>> the worker currently being freed - will be lost.  Please correct me if
>> I'm wrong.
> 
> Yep, your right. But what will be the issue of loosing them?
> 
> IIUC we are not loosing any data, just avoiding some works that will be 
> handled later when/if will set a new owner.
>

But will it actually be handled?

vhost_transport_send_pkt()  // called on every packet send
  virtio_vsock_skb_queue_tail(&send_pkt_queue, skb) // add skb to list
  vhost_vq_work_queue(&send_pkt_work) // try to arm the work
    vhost_worker_queue()
      if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
        llist_add(&worker->work_list)
      }

So send_pkt_queue is a list of skbs, it lives on the vhost_vsock device
state, and survives RESET_OWNER.  In that sense you're probably right
that we aren't loosing any data.

There's also send_pkt_work object, also living on the vhost_vsock device
state.  So we're accumulating skbs, and then send_pkg_work gets put in
the worker task list - but only if it's NOT already armed in there, i.e.
QUEUED bit is unset.  And the bit gets cleared by the workload callback
- for vhost_task backend it's vhost_run_work_list().

The most important thing is WHERE this piece of work is being put.  That
is worker->work_list - this list does not survive RESET_OWNER, as we
free the worker in vhost_workers_free().

Now imagine we have RESET_OWNER racing with send_pkt.  In
vhost_workers_free() we acquire ptr to a worker but not NULL'ify it yet.
 Then on the send_pkt path we arm the send_pkt_work, set the QUEUED bit,
and place it on the work_list of a DYING worker.  Then the worker gets
freed.  Now we have send_pkt_work (a singleton struct) with QUEUED set
in its flags, and with no worker to walk through this piece of work and
clear this flag.  As a result - send_pkt_work can't be placed in the
list of any other worker, because it doesn't pass the "if
(!test_and_set_bit(QUEUED)" check.  Thus no new packets can be
processed, and the connection is stalled.

Does this make sense?
>>
>> That said, I agree that vhost_run_work_list() will only work with
>> vhost_task backend, not with kthreads backend.  If we do
>> vhost_worker_flush() instead - I guess it'll keep the drain here, yet
>> become backend-agnostic. I.e.:
>>
>>> +       if (!xa_empty(&dev->worker_xa)) {
>>> +               synchronize_rcu();
>>> +               xa_for_each(&dev->worker_xa, i, worker)
>>> +                       vhost_worker_flush(worker);
>>> +       }
>>
>> With the last 2 lines being equivalent to just calling
>> vhost_dev_flush(dev).  And once we become backend-agnostic here, I'm
>> guessing the warning reported by Sashiko should be dealt with as well.
> 
> I'd avoid `if !xa_empty(&dev->worker_xa)` at all, and call 
> synchronize_rcu() in any case.
>

Agreed.

> About vhost_dev_flush(), we are calling it in several places, and maybe 
> we should re-check them. E.g. we call in vhost_vsock_flush(), but it's 
> also called by vhost_dev_stop(), maybe we can avoid to call 
> vhost_vsock_flush() if we call vhost_dev_stop().
> 
> I'm not sure we really need another one here, but if you think some 
> other works can be queued between the vhost_dev_stop() and the 
> synchronize_rcu() we are adding here, then okay, it may have sense.
>

Note that in our particular case we're gonna do:

vhost_workers_free()
  vhost_dev_flush()  // the flush we're planning to add
  xa_for_each(&dev->worker_xa, i, worker)
    vhost_worker_destroy(dev, worker)
  xa_destroy(&dev->worker_xa)

So we walk through the XArray, destroy workers in it one by one, then
destroy the XArray itself.  Then the next time we call
vhost_dev_flush(), e.g. from vhost_dev_stop() or wherever else, it tries
iterating over the XArray which no longer exists - which is gonna be a
no-op.

Now, we can reach vhost_workers_free() via (at least) 2 paths:
RESET_OWNER and device release path.  On the former the flush is needed
as I illustrated above.  On the latter it's indeed redundant but is
cheap as it's a no-op.

Andrey
> Thanks,
> Stefano
> 

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

[Stefano Garzarella]

On Thu, Jul 16, 2026 at 09:01:22PM +0300, Andrey Drobyshev wrote:
>On 7/16/26 7:13 PM, Stefano Garzarella wrote:
>> On Thu, Jul 16, 2026 at 06:39:48PM +0300, Andrey Drobyshev wrote:
>>> On 7/16/26 11:57 AM, Stefano Garzarella wrote:
>>>> On Tue, Jul 14, 2026 at 06:16:37PM +0300, Andrey Drobyshev wrote:
>>>>> vhost_vq_work_queue() only holds the RCU read lock while it dereferences
>>>>> vq->worker and queues work on it.  vhost_workers_free() however clears
>>>>> the vq->worker pointers and immediately frees the workers, without
>>>>> waiting for a grace period.  A caller that fetched the worker right
>>>>> before the pointer was cleared can therefore still be queueing work on
>>>>> it while it is freed.  And even when the queueing itself wins the race,
>>>>> the work is never run, so its VHOST_WORK_QUEUED bit stays set and all
>>>>> future attempts to queue it are silently skipped.
>>>>>
>>>>> None of the current callers can actually hit this: net and scsi stop
>>>>> their virtqueues before the workers are freed, and vsock unhashes the
>>>>> device and does synchronize_rcu() of its own in vhost_vsock_dev_release()
>>>>> before the workers go away.  But the upcoming VHOST_RESET_OWNER support
>>>>> in vhost-vsock keeps the device hashed while its workers are freed, so
>>>>> the lockless send/cancel paths become able to race with the teardown.
>>>>>
>>>>> Close this the way vhost_worker_killed() already does: clear the
>>>>> vq->worker pointers, wait for a grace period, run whatever the last
>>>>> readers may have queued, and only then free the workers.  The
>>>>> synchronize_rcu() is skipped if the device has no workers, so cleanup of
>>>>> devices which never got an owner stays cheap.
>>>>>
>>>>
>>>> Do we need a Fixes tag for this?
>>>>
>>>
>>> I'm guessing it should be:
>>>
>>> Fixes: 228a27cf78af ("vhost: Allow worker switching while work is queueing")
>>>
>>>> Thanks for pointing out that the issue wasn't occurring, but I think we
>>>> should add it because it's a sneaky problem we discovered by chance.
>>>> IMO the code should already have `synchronize_rcu()` after
>>>> `rcu_assign_pointer()` loop.
>>>>
>>>> @Michael, what do you think?
>>>>
>>>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>>>>> ---
>>>>> drivers/vhost/vhost.c | 15 +++++++++++++++
>>>>> 1 file changed, 15 insertions(+)
>>>>>
>>>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>>>>> index 4c525b3e16ea..0d1414d40f4e 100644
>>>>> --- a/drivers/vhost/vhost.c
>>>>> +++ b/drivers/vhost/vhost.c
>>>>> @@ -729,6 +729,21 @@ static void vhost_workers_free(struct vhost_dev *dev)
>>>>>
>>>>> 	for (i = 0; i < dev->nvqs; i++)
>>>>> 		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
>>>>> +
>>>>> +	/*
>>>>> +	 * vhost_vq_work_queue() reads vq->worker under rcu_read_lock(), so a
>>>>> +	 * caller that fetched a worker before we cleared the pointers above
>>>>> +	 * may still be about to queue work on it.  Wait for those RCU readers
>>>>> +	 * to finish before freeing the worker, then run whatever they queued
>>>>> +	 * so nothing is left with VHOST_WORK_QUEUED set.  Mirrors
>>>>> +	 * vhost_worker_killed().
>>>>> +	 */
>>>>> +	if (!xa_empty(&dev->worker_xa)) {
>>>>> +		synchronize_rcu();
>>>>> +		xa_for_each(&dev->worker_xa, i, worker)
>>>>> +			vhost_run_work_list(worker);
>>>>> +	}
>>>>> +
>>>>
>>>> Following sashiko review [1], I tried to undersand why we need this, but
>>>> TBH I'm really confused. That said, this seems wrong also because it
>>>> will work only with vhost_tasks, and not with kthreads.
>>>>
>>>> IIUC vhost_worker_killed() will be called anyway when calling
>>>> vhost_worker_destroy(). For vhost_tasks, it will call
>>>> vhost_task_do_stop() that calls vhost_task_stop(). This sets
>>>> VHOST_TASK_FLAGS_STOP and wait the worker on vtsk->exited before freeing
>>>> stuff. The worker breaks the loop and calls vtsk->handle_sigkill() that
>>>> is exactly vhost_worker_killed() you mentioned we are mirroring here.
>>>>
>>>
>>> Hmm, are we sure it's the case for our codepath?  Looking at the
>>> vhost_task loop function:
>>>
>>>> static int vhost_task_fn(void *data)
>>>> {
>>>>     for (;;) {
>>>>         if (signal_pending(current)) {
>>>>             if (get_signal(&ksig))
>>>>                 break;
>>>>         }
>>>>         ...
>>>>         if (test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
>>>>             __set_current_state(TASK_RUNNING);
>>>>             break;
>>>>         }
>>>>         did_work = vtsk->fn(vtsk->data);
>>>>         ...
>>>>     }
>>>>
>>>>     ...
>>>>
>>>>     if (!test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
>>>>         set_bit(VHOST_TASK_FLAGS_KILLED, &vtsk->flags);
>>>>         vtsk->handle_sigkill(vtsk->data);
>>>>     }
>>>>     ...
>>>> }
>>>
>>> AFAICT, we exit the loop in 2 cases: signal delivery or STOP bit
>>> setting.  Like you said, STOP is set by vhost_task_stop.  E.g. for our
>>> RESET_OWNER case:
>>>
>>> vhost_vsock_reset_owner()
>>>  vhost_dev_reset_owner()
>>>    vhost_dev_cleanup()
>>>      vhost_workers_free()
>>>        vhost_worker_destroy()
>>>          vhost_task_stop()  // for vhost_task_ops backend
>>>            set_bit(VHOST_TASK_FLAGS_STOP)
>>>
>>> So, first of all, actual work by .fn() callback is done after the exit
>>> checks, therefore we skip it - no chance to drain there.
>>>
>>> Secondly, the handle_sigkill() callback is deliberately NOT called in
>>> the STOP case and only called on fatal signal delivery.  And for
>>> vhost_task backend the .handle_sigkill() callback is exactly
>>> vhost_worker_killed().
>>>
>>> So my understanding is: if we only call synchronize_rcu() here and leave
>>> this path undrained, then whatever work which was put by send_pkt() for
>>> the worker currently being freed - will be lost.  Please correct me if
>>> I'm wrong.
>>
>> Yep, your right. But what will be the issue of loosing them?
>>
>> IIUC we are not loosing any data, just avoiding some works that will be
>> handled later when/if will set a new owner.
>>
>
>But will it actually be handled?
>
>vhost_transport_send_pkt()  // called on every packet send
>  virtio_vsock_skb_queue_tail(&send_pkt_queue, skb) // add skb to list
>  vhost_vq_work_queue(&send_pkt_work) // try to arm the work
>    vhost_worker_queue()
>      if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
>        llist_add(&worker->work_list)
>      }
>
>So send_pkt_queue is a list of skbs, it lives on the vhost_vsock device
>state, and survives RESET_OWNER.  In that sense you're probably right
>that we aren't loosing any data.
>
>There's also send_pkt_work object, also living on the vhost_vsock device
>state.  So we're accumulating skbs, and then send_pkg_work gets put in
>the worker task list - but only if it's NOT already armed in there, i.e.
>QUEUED bit is unset.  And the bit gets cleared by the workload callback
>- for vhost_task backend it's vhost_run_work_list().
>
>The most important thing is WHERE this piece of work is being put.  That
>is worker->work_list - this list does not survive RESET_OWNER, as we
>free the worker in vhost_workers_free().
>
>Now imagine we have RESET_OWNER racing with send_pkt.  In
>vhost_workers_free() we acquire ptr to a worker but not NULL'ify it yet.
> Then on the send_pkt path we arm the send_pkt_work, set the QUEUED bit,
>and place it on the work_list of a DYING worker.  Then the worker gets
>freed.  Now we have send_pkt_work (a singleton struct) with QUEUED set
>in its flags, and with no worker to walk through this piece of work and
>clear this flag.  As a result - send_pkt_work can't be placed in the
>list of any other worker, because it doesn't pass the "if
>(!test_and_set_bit(QUEUED)" check.  Thus no new packets can be
>processed, and the connection is stalled.
>
>Does this make sense?

Honestly, I don't see the problem. The device is about to be stopped 
because the VMM is resetting the owner, so the connection is going to 
stall anyway, since I don't think the guest will never answer, isn't it?

>>>
>>> That said, I agree that vhost_run_work_list() will only work with
>>> vhost_task backend, not with kthreads backend.  If we do
>>> vhost_worker_flush() instead - I guess it'll keep the drain here, yet
>>> become backend-agnostic. I.e.:
>>>
>>>> +       if (!xa_empty(&dev->worker_xa)) {
>>>> +               synchronize_rcu();
>>>> +               xa_for_each(&dev->worker_xa, i, worker)
>>>> +                       vhost_worker_flush(worker);
>>>> +       }
>>>
>>> With the last 2 lines being equivalent to just calling
>>> vhost_dev_flush(dev).  And once we become backend-agnostic here, I'm
>>> guessing the warning reported by Sashiko should be dealt with as well.
>>
>> I'd avoid `if !xa_empty(&dev->worker_xa)` at all, and call
>> synchronize_rcu() in any case.
>>
>
>Agreed.
>
>> About vhost_dev_flush(), we are calling it in several places, and maybe
>> we should re-check them. E.g. we call in vhost_vsock_flush(), but it's
>> also called by vhost_dev_stop(), maybe we can avoid to call
>> vhost_vsock_flush() if we call vhost_dev_stop().
>>
>> I'm not sure we really need another one here, but if you think some
>> other works can be queued between the vhost_dev_stop() and the
>> synchronize_rcu() we are adding here, then okay, it may have sense.
>>
>
>Note that in our particular case we're gonna do:
>
>vhost_workers_free()
>  vhost_dev_flush()  // the flush we're planning to add
>  xa_for_each(&dev->worker_xa, i, worker)
>    vhost_worker_destroy(dev, worker)
>  xa_destroy(&dev->worker_xa)
>
>So we walk through the XArray, destroy workers in it one by one, then
>destroy the XArray itself.  Then the next time we call
>vhost_dev_flush(), e.g. from vhost_dev_stop() or wherever else, it tries
>iterating over the XArray which no longer exists - which is gonna be a
>no-op.
>
>Now, we can reach vhost_workers_free() via (at least) 2 paths:
>RESET_OWNER and device release path.  On the former the flush is needed
>as I illustrated above.  On the latter it's indeed redundant but is
>cheap as it's a no-op.

Again, I don't see the need for it TBH, but at the same time, I don't 
think it's a problem to include it, so if you think it's necessary, go 
ahead and add it.

Thanks,
Stefano

[PATCH v4 5/5] vhost/vsock: add VHOST_RESET_OWNER ioctl

[Andrey Drobyshev]

From: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

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.

To that end, unlike the release path, RESET_OWNER keeps the guest CID
hashed: established connections survive, and host sends issued while
the device is between owners simply stay on send_pkt_queue until the
next device start drains them.

Since the device stays reachable through the CID hash, the lockless
send/cancel paths can race with the worker teardown in
vhost_workers_free().  The previous commit ("vhost: synchronize with
RCU readers when freeing workers") makes that safe.

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 drivers/vhost/vsock.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index d5022d21120b..86f25ff80722 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -903,6 +903,29 @@ static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
 	return -EFAULT;
 }
 
+static long 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;
+	}
+	vhost_vsock_drop_backends(vsock);
+	vhost_vsock_flush(vsock);
+	vhost_dev_stop(&vsock->dev);
+	vhost_dev_reset_owner(&vsock->dev, umem);
+done:
+	mutex_unlock(&vsock->dev.mutex);
+	return err;
+}
+
 static long vhost_vsock_dev_ioctl(struct file *f, unsigned int ioctl,
 				  unsigned long arg)
 {
@@ -946,6 +969,8 @@ static long vhost_vsock_dev_ioctl(struct file *f, unsigned int ioctl,
 			return -EOPNOTSUPP;
 		vhost_set_backend_features(&vsock->dev, features);
 		return 0;
+	case VHOST_RESET_OWNER:
+		return vhost_vsock_reset_owner(vsock);
 	default:
 		mutex_lock(&vsock->dev.mutex);
 		r = vhost_dev_ioctl(&vsock->dev, ioctl, argp);
-- 
2.47.1