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

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 <[email protected]>
>> Signed-off-by: Andrey Drobyshev <[email protected]>
>> ---
>> 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/[email protected]?part=4
>