Re: [Devel] [PATCH hci-8.1 1/2] vfio: move an open device's kvm reference when its group gets a new kvm

[Konstantin Khorenko <khorenko@virtuozzo.com>]

I won't apply this version, please send v2 with the changes below.

On Fri, 30 Oct 2026 16:49:32 +0200, Andrey Drobyshev wrote:
> +	mutex_lock(&group->device_lock);
> +	list_for_each_entry(device, &group->device_list, group_next) {
> +		mutex_lock(&device->dev_set->lock);
> +		if (device->kvm && device->kvm != kvm) {
> +			vfio_device_put_kvm(device);
> +			vfio_device_get_kvm_safe(device, kvm);
> +		}

Item 1 of the cover letter reply: do not change device->kvm under a
driver that still uses it after open_device(). Check the NVIDIA vGPU
driver and say in the message which driver this was tested with.

Why: until now device->kvm did not change between open_device() and
close_device(), and drivers rely on that. KVMGT registers its
page-track notifier on device->kvm at open, and that takes a reference
on the kvm. At close it unregisters from device->kvm, which drops a
reference. After the move the close drops a reference on the new VM
that it never took, so the new VM is freed while QEMU still uses it.
KVMGT is not built in vz10, but the vGPU driver from the ticket is out
of tree and may do the same.

> +	/*
> +	 * Open devices hold a reference to the kvm they were opened with.
> +	 * Move it to the new kvm, or the old one stays pinned until the
> +	 * device is closed.  Nothing to move to on NULL.
> +	 */
> +	if (!kvm)
> +		return;

Item 2 of the cover letter reply: say in this comment why NULL is
skipped: the lock order. Update the kernel-doc of vfio_file_set_kvm()
too, it says the device gets the kvm when it is first opened.

Why: the NULL call from kvm_device_release() comes with kvm->lock held.
vfio_ap takes kvm->lock in its open_device(), which runs under
dev_set->lock. Taking dev_set->lock here for NULL would close an ABBA
deadlock of the kind 2b48f52f2bff fixed. "Nothing to move to" is not the
reason.

> Move the reference to the new kvm.  Leave NULL alone: that is the old
> kvm-vfio device going away under kvm->lock, and the new kvm, if any,
> arrives on its own.

Item 2 of the cover letter reply: drop "and the new kvm, if any,
arrives on its own" and give the lock order as the reason instead.

Why: with cpr-transfer the new QEMU adds the group to its kvm-vfio
first, and the old QEMU's NULL comes last. It leaves group->kvm NULL.
The open devices keep the new kvm, so the code is fine, the sentence
is not. A NULL from KVM_DEV_VFIO_FILE_DEL also holds only kv->lock,
not kvm->lock.

> https://virtuozzo.atlassian.net/browse/VSTOR-150597

Item 3 of the cover letter reply: add a Fixes tag for 2b48f52f2bff
("vfio: fix deadlock between group lock and kvm lock").

Why: that commit made the device hold a reference to the kvm until its
last close.

Item 6 of the cover letter reply: single space after a period, here
and in the code comment, and the Feature: line.

This needs to go to mainline too, I created a task for that:
https://virtuozzo.atlassian.net/browse/VSTOR-153437

Mainline vfio_group_set_kvm() also swaps only group->kvm, and since
5c3ea6d3e836d the open device even holds the old VM's file. The change
of the device->kvm contract is something the vfio maintainers have to
agree to.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel