On 10/8/26 5:12 PM, Akihiko Odaki wrote: > On 2026/10/08 17:37, Andrey Drobyshev wrote: >> On 10/7/26 6:37 PM, Akihiko Odaki wrote: >>> On 2026/10/07 17:40, Andrey Drobyshev wrote: >>>> vhost_dev_cleanup() closes the backend FD, but that doesn't release the >>>> device ownership taken with VHOST_SET_OWNER if the same file is open >>>> elsewhere. That's the case on a CPR target: the source keeps the FD, >>>> so if the target fails after SET_OWNER, either inside vhost_dev_init() >>>> or later in its caller, the ownership stays with the target. Once the >>>> source is resumed it can't take the device back, SET_OWNER fails with >>>> EBUSY. >>>> >>>> Track the ownership with a new 'owner' flag in struct vhost_dev, set and >>>> cleared by vhost_dev_set_owner() and vhost_dev_reset_owner(), and issue >>>> VHOST_RESET_OWNER in vhost_dev_cleanup() if it's set, as the common >>>> teardown for all those paths. That should be safe in any case: the kernel >>>> refuses it from a non-owner, and an owner is about to close the FD anyway. >>>> >>>> Reported-by: Akihiko Odaki <[email protected]> >>>> Suggested-by: Stefano Garzarella <[email protected]> >>>> Signed-off-by: Andrey Drobyshev <[email protected]> >>>> --- >>>> hw/virtio/vhost.c | 24 ++++++++++++++++++++++-- >>>> include/hw/virtio/vhost.h | 5 +++++ >>>> 2 files changed, 27 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >>>> index c01ccc800cb..3432b9a43c8 100644 >>>> --- a/hw/virtio/vhost.c >>>> +++ b/hw/virtio/vhost.c >>>> @@ -1705,20 +1705,32 @@ int vhost_dev_init_backend(struct vhost_dev *hdev, >>>> void *opaque, >>>> >>>> int vhost_dev_set_owner(struct vhost_dev *hdev) >>>> { >>>> + int r; >>>> + >>>> assert(hdev->vhost_ops); >>>> if (!hdev->vhost_ops->vhost_set_owner) { >>>> return -ENOSYS; >>>> } >>>> - return hdev->vhost_ops->vhost_set_owner(hdev); >>>> + r = hdev->vhost_ops->vhost_set_owner(hdev); >>>> + if (!r) { >>>> + hdev->owner = true; >>>> + } >>>> + return r; >>>> } >>>> >>>> int vhost_dev_reset_owner(struct vhost_dev *hdev) >>>> { >>>> + int r; >>>> + >>>> assert(hdev->vhost_ops); >>>> if (!hdev->vhost_ops->vhost_reset_owner) { >>>> return -ENOSYS; >>>> } >>>> - return hdev->vhost_ops->vhost_reset_owner(hdev); >>>> + r = hdev->vhost_ops->vhost_reset_owner(hdev); >>>> + if (!r) { >>>> + hdev->owner = false; >>>> + } >>>> + return r; >>>> } >>>> >>>> int vhost_dev_init(struct vhost_dev *hdev, void *opaque, >>>> @@ -1864,6 +1876,14 @@ void vhost_dev_cleanup(struct vhost_dev *hdev) >>>> g_free(hdev->mem); >>>> g_free(hdev->mem_sections); >>>> if (hdev->vhost_ops) { >>>> + /* >>>> + * Release the device ownership if we hold it. Needed e.g. for >>>> + * failing CPR target, so the source can re-acquire it. >>>> + */ >>>> + if (hdev->owner && vhost_dev_reset_owner(hdev) < 0) { >>>> + error_report("vhost: cannot release device ownership on " >>>> + "cleanup"); >>> >>> Normal teardown produces a misleading ownership error. vhost-user and >>> vDPA set owner=true but do not implement vhost_reset_owner, so the >>> helper returns -ENOSYS. The new cleanup diagnostic therefore reports >>> ordinary cleanup as a failure. I reproduced the message with the >>> following test command even though it passes: >>> >>> meson test -C build --no-rebuild -v \ >>> --suite qtest-x86_64 '*qos-test*' \ >>> --test-args '-p >>> /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-net-pci/virtio-net/virtio-net-tests/vhost-user/read-guest-mem/memfd' >>> >>> Regards, >>> Akihiko Odaki >> Ah, looks you're right again. I just noticed that the error goes silent >> here and we better somehow flag it, even though we can't do anything >> about it at this point. I guess we can just ignore the cases where >> either the backend doesn't have the reset_owner() callback (in this case >> we get -ENOSYS) and when the kernel driver itself doesn't support >> RESET_OWNER operation (in this case we get -ENOTTY). So smth like: >> >> r = vhost_dev_reset_owner(hdev); >> if (r < 0 && r != -ENOSYS && r != -ENOTTY) { >> error_report(...); >> } >> >> I'll post this if you don't have any objections. > > The -ENOSYS filtering fixes what I reported, but this check misses a > kernel quirk: unsupported VHOST_RESET_OWNER can return -EFAULT. > > For example, Linux v6.12’s vhost-vsock forwards unknown requests through > vhost_dev_ioctl() to vhost_vring_ioctl(). The latter reads a queue > index from the argument before checking the command. QEMU passes NULL, > so that read fails with -EFAULT. QEMU already documents this quirk in > vhost_kernel_set_features(). > > I'm a bit concerned that this check may not be reliable enough and > result in false-positives/negatives or a behavioral change in the > future. Perhaps having a flag to tell that devices need explicit > ownership release for CPR may be an option. The source must have > successfully reset ownership on the same FD before the target acquires > it. A reset failure on the target therefore deserves reporting, > regardless of errno. > > Regards, > Akihiko Odaki
I think we might indeed get away with flags here. However your remark made me look into the code once more and I think 'vsock->owner_reset' flag is currently redundant. At each meaningful point in the code this flag has the reverse value of 'vvc->vhost_dev.owner', i.e. base class 'owner' flag. So I believe we should be fine with only 2 ownership flags instead of 3, both in the base 'struct vhost_dev': * 'owner': explicitly set by set_owner() / reset_owner(); * 'owner_handed_over': only ever set in .post_load(), on CPR target. And then in try_release_owner() we guard by both flags (owner && owner_handed_over). That way ownership release will only be attempted by CPR target. And this way reset failure will be reported regardless of errno. Do you think this'll be acceptable? Thanks, Andrey
