On 9/30/26 2:58 PM, Stefano Garzarella wrote:
> On Wed, Sep 30, 2026 at 01:13:14PM +0300, Andrey Drobyshev wrote:
>> vhost_dev_init() takes the device ownership with VHOST_SET_OWNER and
>> then may still fail, e.g. on virtqueue init. Its cleanup only closes
>> the backend fd, which doesn't release the ownership if the same file
>> is open elsewhere. That's the case on a CPR target: the source keeps
>> the fd, and once it's resumed after the failed migration it can't take
>> the device back, SET_OWNER fails with EBUSY.
>>
>> Release the ownership explicitly on the failure paths past SET_OWNER.
>>
>> Reported-by: Akihiko Odaki <[email protected]>
>> Signed-off-by: Andrey Drobyshev <[email protected]>
>> ---
>> hw/virtio/vhost.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> If you have to send another version, maybe better to move the
> introduction of vhost_dev_reset_owner() in this patch since IIUC this is
> the first place where we use it. (not a strong opinion)
I'd still prefer separating them: one is pure API addition, subsequent
ones #15 and #16 are actual functional change (also not a strong opinion
here).
> That said, what happen if the caller of vhost_dev_init() fails after
> calling it? Who will call vhost_dev_reset_owner()?
>
> E.g. in vhost_vsock_post_load(), if vhost_vsock_set_guest_cid() fails,
> should we call vhost_dev_reset_owner()?
>
> Since IIUC all error paths call vhost_dev_cleanup(), might it make sense
> to call vhost_dev_reset_owner() there?
I think you're exactly right and it makes sense to issue RESET_OWNER
directly from vhost_dev_cleanup(). Thanks for spotting the bug!
By doing so, we obviously suggest that unconditional RESET_OWNER should
be safe for any other non-CPR case. In particular, on the cleanup path
we call
vhost_dev_cleanup() ->
hdev->vhost_ops->vhost_cleanup() ->
vhost_kernel_cleanup() -> // kernel backend
close(vhostfd)
So backend FD is being closed either way. IMHO that alone should be
enough to assume that resetting owner for any vhost-backed device is
safe. Please correct me if I'm wrong here.
Thanks,
Andrey
>
> Thanks,
> Stefano
>
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index c01ccc800cb..3ff54879068 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1754,7 +1754,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void
>> *opaque,
>> error_append_hint(errp, "Try plugging this vhost backend before"
>> " plugging such memory devices.\n");
>> r = -EINVAL;
>> - goto fail;
>> + goto fail_owner;
>> }
>>
>> for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
>> @@ -1762,7 +1762,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void
>> *opaque,
>> busyloop_timeout);
>> if (r < 0) {
>> error_setg_errno(errp, -r, "Failed to initialize virtqueue %d",
>> i);
>> - goto fail;
>> + goto fail_owner;
>> }
>> }
>>
>> @@ -1799,7 +1799,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void
>> *opaque,
>> if (hdev->migration_blocker != NULL) {
>> r = migrate_add_blocker_normal(&hdev->migration_blocker, errp);
>> if (r < 0) {
>> - goto fail;
>> + goto fail_owner;
>> }
>> }
>>
>> @@ -1831,7 +1831,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void
>> *opaque,
>> " than current number of used (%d) and reserved (%d)"
>> " memory slots for memory devices.", limit, used,
>> reserved);
>> r = -EINVAL;
>> - goto fail;
>> + goto fail_owner;
>> }
>>
>> hdev->initialized = true;
>> @@ -1840,6 +1840,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void
>> *opaque,
>>
>> return 0;
>>
>> +fail_owner:
>> + vhost_dev_reset_owner(hdev);
>> fail:
>> hdev->nvqs = n_initialized_vqs;
>> vhost_dev_cleanup(hdev);
>>
>> --
>> 2.47.1
>>
>
