On Wed, Sep 30, 2026 at 05:47:11PM +0300, Andrey Drobyshev wrote:
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).
I usually prefer to avoid commits with unused functions, but I see your
point, and this approach makes sense too, so I have no objections ;-)
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.
Yeah, it seems correct to me too.
If we want to be more cautious, we could pass a parameter to
vhost_dev_init() (or add a new function that calls vhost_dev_init) to
store a variable in vhost_dev that indicates whether the device supports
CPR, and decide whether or not to call vhost_dev_reset_owner() during
cleanup. But again, I think we are fine calling vhost_dev_reset_owner()
unconditionally.
Thanks,
Stefano
