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.
Thanks,
Andrey
