On 2026/10/09 19:14, Andrey Drobyshev wrote:
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?
Yes, provided owner_handed_over is set before the vhost_dev_init().
vhost_dev_init() can acquire ownership and then fail through its own
cleanup. Setting the flag after successful vhost_dev_init() would miss
that failure path.
Removing owner_reset also looks reasonable: use owner in .pre_save() and
!owner when restarting.
With that ordering, owner && owner_handed_over identifies when
explicit release is needed, and reporting every reset failure avoids
the unreliable errno filtering.
Regards,
Akihiko Odaki
