On Fri, 20 Jan 2023 07:05:28 -0800
Yi Liu <yi.l.liu@xxxxxxxxx> wrote:
> Currently it is possible that the final put of a KVM reference comes from
> vfio during its device close operation. This occurs while the vfio group
> lock is held; however, if the vfio device is still in the kvm device list,
> then the following call chain could result in a deadlock:
>
> VFIO holds group->group_lock/group_rwsem
> -> kvm_put_kvm
> -> kvm_destroy_vm
> -> kvm_destroy_devices
> -> kvm_vfio_destroy
> -> kvm_vfio_file_set_kvm
> -> vfio_file_set_kvm
> -> try to hold group->group_lock/group_rwsem
>
> The key function is the kvm_destroy_devices() which triggers destroy cb
> of kvm_device_ops. It calls back to vfio and try to hold group_lock. So
> if this path doesn't call back to vfio, this dead lock would be fixed.
> Actually, there is a way for it. KVM provides another point to free the
> kvm-vfio device which is the point when the device file descriptor is
> closed. This can be achieved by providing the release cb instead of the
> destroy cb. Also rename kvm_vfio_destroy() to be kvm_vfio_release().
>
> /*
> * Destroy is responsible for freeing dev.
> *
> * Destroy may be called before or after destructors are called
> * on emulated I/O regions, depending on whether a reference is
> * held by a vcpu or other kvm component that gets destroyed
> * after the emulated I/O.
> */
> void (*destroy)(struct kvm_device *dev);
>
> /*
> * Release is an alternative method to free the device. It is
> * called when the device file descriptor is closed. Once
> * release is called, the destroy method will not be called
> * anymore as the device is removed from the device list of
> * the VM. kvm->lock is held.
> */
> void (*release)(struct kvm_device *dev);
>
> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
> Reported-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Suggested-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> ---
> virt/kvm/vfio.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 495ceabffe88..e94f3ea718e5 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -336,7 +336,7 @@ static int kvm_vfio_has_attr(struct kvm_device *dev,
> return -ENXIO;
> }
>
> -static void kvm_vfio_destroy(struct kvm_device *dev)
> +static void kvm_vfio_release(struct kvm_device *dev)
> {
> struct kvm_vfio *kv = dev->private;
> struct kvm_vfio_group *kvg, *tmp;
> @@ -363,7 +363,7 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type);
> static struct kvm_device_ops kvm_vfio_ops = {
> .name = "kvm-vfio",
> .create = kvm_vfio_create,
> - .destroy = kvm_vfio_destroy,
> + .release = kvm_vfio_release,
> .set_attr = kvm_vfio_set_attr,
> .has_attr = kvm_vfio_has_attr,
> };
Applied to vfio for-linus branch for v6.2, along with Matthew's R-b,
the comment update, and the extra reference link. Once we get a
linux-next build I'll send a pull request, along with Matthew's
reserved region fix. Thanks,
Alex
