[v2] vfio: fix potential deadlock on vfio group lock

  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:

kvm_put_kvm
 -> kvm_destroy_vm
  -> kvm_destroy_devices
   -> kvm_vfio_destroy
    -> kvm_vfio_file_set_kvm
     -> vfio_file_set_kvm
      -> group->group_lock/group_rwsem

Avoid this scenario by having vfio core code acquire a KVM reference
the first time a device is opened and hold that reference until the
device fd is closed, at a point after the group lock has been released.

Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
Reported-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
Changes from v1:
* Re-write using symbol get logic to get kvm ref during first device
  open, release the ref during device fd close after group lock is
  released
* Drop kvm get/put changes to drivers; now that vfio core holds a
  kvm ref until sometime after the device_close op is called, it
  should be fine for drivers to get and put their own references to it.
---
 drivers/vfio/group.c     |  6 ++---
 drivers/vfio/vfio_main.c | 48 +++++++++++++++++++++++++++++++++++++---
 include/linux/vfio.h     |  1 -
 3 files changed, 48 insertions(+), 7 deletions(-)
  

On Thu, 12 Jan 2023 15:38:44 -0500
Matthew Rosato <mjrosato@linux.ibm.com> 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:
> 
> kvm_put_kvm
>  -> kvm_destroy_vm
>   -> kvm_destroy_devices
>    -> kvm_vfio_destroy
>     -> kvm_vfio_file_set_kvm
>      -> vfio_file_set_kvm
>       -> group->group_lock/group_rwsem  
> 
> Avoid this scenario by having vfio core code acquire a KVM reference
> the first time a device is opened and hold that reference until the
> device fd is closed, at a point after the group lock has been released.
> 
> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
> Changes from v1:
> * Re-write using symbol get logic to get kvm ref during first device
>   open, release the ref during device fd close after group lock is
>   released
> * Drop kvm get/put changes to drivers; now that vfio core holds a
>   kvm ref until sometime after the device_close op is called, it
>   should be fine for drivers to get and put their own references to it.
> ---
>  drivers/vfio/group.c     |  6 ++---
>  drivers/vfio/vfio_main.c | 48 +++++++++++++++++++++++++++++++++++++---
>  include/linux/vfio.h     |  1 -
>  3 files changed, 48 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> index bb24b2f0271e..2b0da82f82f4 100644
> --- a/drivers/vfio/group.c
> +++ b/drivers/vfio/group.c
> @@ -165,9 +165,9 @@ static int vfio_device_group_open(struct vfio_device *device)
>  	}
>  
>  	/*
> -	 * Here we pass the KVM pointer with the group under the lock.  If the
> -	 * device driver will use it, it must obtain a reference and release it
> -	 * during close_device.
> +	 * Here we pass the KVM pointer with the group under the lock.  A
> +	 * reference will be obtained the first time the device is opened and
> +	 * will be held until the device fd is closed.
>  	 */
>  	ret = vfio_device_open(device, device->group->iommufd,
>  			       device->group->kvm);
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 5177bb061b17..c969e2a0ecd3 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -16,6 +16,7 @@
>  #include <linux/fs.h>
>  #include <linux/idr.h>
>  #include <linux/iommu.h>
> +#include <linux/kvm_host.h>
>  #include <linux/list.h>
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
> @@ -344,6 +345,35 @@ static bool vfio_assert_device_open(struct vfio_device *device)
>  	return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
>  }
>  
> +static bool vfio_kvm_get_kvm_safe(struct kvm *kvm)
> +{
> +	bool (*fn)(struct kvm *kvm);
> +	bool ret;
> +
> +	fn = symbol_get(kvm_get_kvm_safe);
> +	if (WARN_ON(!fn))
> +		return false;
> +
> +	ret = fn(kvm);
> +
> +	symbol_put(kvm_get_kvm_safe);
> +
> +	return ret;
> +}
> +
> +static void vfio_kvm_put_kvm(struct kvm *kvm)
> +{
> +	void (*fn)(struct kvm *kvm);
> +
> +	fn = symbol_get(kvm_put_kvm);
> +	if (WARN_ON(!fn))
> +		return;
> +
> +	fn(kvm);
> +
> +	symbol_put(kvm_put_kvm);
> +}
> +
>  static int vfio_device_first_open(struct vfio_device *device,
>  				  struct iommufd_ctx *iommufd, struct kvm *kvm)
>  {
> @@ -361,16 +391,24 @@ static int vfio_device_first_open(struct vfio_device *device,
>  	if (ret)
>  		goto err_module_put;
>  
> +	if (kvm && !vfio_kvm_get_kvm_safe(kvm)) {
> +		ret = -ENOENT;
> +		goto err_unuse_iommu;
> +	}
>  	device->kvm = kvm;

This could just as easily be:

	if (kvm && vfio_kvm_get_kvm_safe(kvm))
		device->kvm = kvm;

Right?  The error path would test device->kvm and we already use
device->kvm in the release path.

Otherwise, in the off chance userspace hits this error, what's the
value in generating a failure here for a device that may or may not
have a kvm dependency.  A driver with a dependency should fail if
device->kvm is NULL.

>  	if (device->ops->open_device) {
>  		ret = device->ops->open_device(device);
>  		if (ret)
> -			goto err_unuse_iommu;
> +			goto err_put_kvm;
>  	}
>  	return 0;
>  
> +err_put_kvm:
> +	if (kvm) {
> +		vfio_kvm_put_kvm(kvm);
> +		device->kvm = NULL;
> +	}
>  err_unuse_iommu:
> -	device->kvm = NULL;
>  	if (iommufd)
>  		vfio_iommufd_unbind(device);
>  	else
> @@ -387,7 +425,6 @@ static void vfio_device_last_close(struct vfio_device *device,
>  
>  	if (device->ops->close_device)
>  		device->ops->close_device(device);
> -	device->kvm = NULL;
>  	if (iommufd)
>  		vfio_iommufd_unbind(device);
>  	else
> @@ -465,6 +502,11 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>  
>  	vfio_device_group_close(device);
>  
> +	if (device->open_count == 0 && device->kvm) {
> +		vfio_kvm_put_kvm(device->kvm);
> +		device->kvm = NULL;
> +	}

IIUC, device->open_count is protected by device->dev_set->lock.  Thanks,

Alex

> +
>  	vfio_device_put_registration(device);
>  
>  	return 0;
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 35be78e9ae57..3ff7e9302cc1 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -46,7 +46,6 @@ struct vfio_device {
>  	struct vfio_device_set *dev_set;
>  	struct list_head dev_set_list;
>  	unsigned int migration_flags;
> -	/* Driver must reference the kvm during open_device or never touch it */
>  	struct kvm *kvm;
>  
>  	/* Members below here are private, not for driver use */
  

On 1/12/23 4:05 PM, Alex Williamson wrote:
> On Thu, 12 Jan 2023 15:38:44 -0500
> Matthew Rosato <mjrosato@linux.ibm.com> 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:
>>
>> kvm_put_kvm
>>  -> kvm_destroy_vm
>>   -> kvm_destroy_devices
>>    -> kvm_vfio_destroy
>>     -> kvm_vfio_file_set_kvm
>>      -> vfio_file_set_kvm
>>       -> group->group_lock/group_rwsem  
>>
>> Avoid this scenario by having vfio core code acquire a KVM reference
>> the first time a device is opened and hold that reference until the
>> device fd is closed, at a point after the group lock has been released.
>>
>> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
>> Reported-by: Alex Williamson <alex.williamson@redhat.com>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>> ---
>> Changes from v1:
>> * Re-write using symbol get logic to get kvm ref during first device
>>   open, release the ref during device fd close after group lock is
>>   released
>> * Drop kvm get/put changes to drivers; now that vfio core holds a
>>   kvm ref until sometime after the device_close op is called, it
>>   should be fine for drivers to get and put their own references to it.
>> ---
>>  drivers/vfio/group.c     |  6 ++---
>>  drivers/vfio/vfio_main.c | 48 +++++++++++++++++++++++++++++++++++++---
>>  include/linux/vfio.h     |  1 -
>>  3 files changed, 48 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
>> index bb24b2f0271e..2b0da82f82f4 100644
>> --- a/drivers/vfio/group.c
>> +++ b/drivers/vfio/group.c
>> @@ -165,9 +165,9 @@ static int vfio_device_group_open(struct vfio_device *device)
>>  	}
>>  
>>  	/*
>> -	 * Here we pass the KVM pointer with the group under the lock.  If the
>> -	 * device driver will use it, it must obtain a reference and release it
>> -	 * during close_device.
>> +	 * Here we pass the KVM pointer with the group under the lock.  A
>> +	 * reference will be obtained the first time the device is opened and
>> +	 * will be held until the device fd is closed.
>>  	 */
>>  	ret = vfio_device_open(device, device->group->iommufd,
>>  			       device->group->kvm);
>> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
>> index 5177bb061b17..c969e2a0ecd3 100644
>> --- a/drivers/vfio/vfio_main.c
>> +++ b/drivers/vfio/vfio_main.c
>> @@ -16,6 +16,7 @@
>>  #include <linux/fs.h>
>>  #include <linux/idr.h>
>>  #include <linux/iommu.h>
>> +#include <linux/kvm_host.h>
>>  #include <linux/list.h>
>>  #include <linux/miscdevice.h>
>>  #include <linux/module.h>
>> @@ -344,6 +345,35 @@ static bool vfio_assert_device_open(struct vfio_device *device)
>>  	return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
>>  }
>>  
>> +static bool vfio_kvm_get_kvm_safe(struct kvm *kvm)
>> +{
>> +	bool (*fn)(struct kvm *kvm);
>> +	bool ret;
>> +
>> +	fn = symbol_get(kvm_get_kvm_safe);
>> +	if (WARN_ON(!fn))
>> +		return false;
>> +
>> +	ret = fn(kvm);
>> +
>> +	symbol_put(kvm_get_kvm_safe);
>> +
>> +	return ret;
>> +}
>> +
>> +static void vfio_kvm_put_kvm(struct kvm *kvm)
>> +{
>> +	void (*fn)(struct kvm *kvm);
>> +
>> +	fn = symbol_get(kvm_put_kvm);
>> +	if (WARN_ON(!fn))
>> +		return;
>> +
>> +	fn(kvm);
>> +
>> +	symbol_put(kvm_put_kvm);
>> +}
>> +
>>  static int vfio_device_first_open(struct vfio_device *device,
>>  				  struct iommufd_ctx *iommufd, struct kvm *kvm)
>>  {
>> @@ -361,16 +391,24 @@ static int vfio_device_first_open(struct vfio_device *device,
>>  	if (ret)
>>  		goto err_module_put;
>>  
>> +	if (kvm && !vfio_kvm_get_kvm_safe(kvm)) {
>> +		ret = -ENOENT;
>> +		goto err_unuse_iommu;
>> +	}
>>  	device->kvm = kvm;
> 
> This could just as easily be:
> 
> 	if (kvm && vfio_kvm_get_kvm_safe(kvm))
> 		device->kvm = kvm;
> 
> Right?  The error path would test device->kvm and we already use
> device->kvm in the release path.

Yeah, with a slight change (see below)

> 
> Otherwise, in the off chance userspace hits this error, what's the
> value in generating a failure here for a device that may or may not
> have a kvm dependency.  A driver with a dependency should fail if
> device->kvm is NULL.

Hmm, you have a point.  Yes, I agree that any driver that needs device->kvm is responsible for checking it for NULL.  I guess I was viewing this case as 'oh, we must already be on the kvm_destroy_vm path for this group' but that just means group->kvm is about to go NULL and doesn't necessarily mean that the vfio group is also going away.

Will change.

> 
>>  	if (device->ops->open_device) {
>>  		ret = device->ops->open_device(device);
>>  		if (ret)
>> -			goto err_unuse_iommu;
>> +			goto err_put_kvm;
>>  	}
>>  	return 0;
>>  
>> +err_put_kvm:
>> +	if (kvm) {

s/kvm/device->kvm/ here to go along with your suggestion above, that way we only do the kvm_put if we previously had a successful kvm_get

>> +		vfio_kvm_put_kvm(kvm);
>> +		device->kvm = NULL;
>> +	}
>>  err_unuse_iommu:
>> -	device->kvm = NULL;
>>  	if (iommufd)
>>  		vfio_iommufd_unbind(device);
>>  	else
>> @@ -387,7 +425,6 @@ static void vfio_device_last_close(struct vfio_device *device,
>>  
>>  	if (device->ops->close_device)
>>  		device->ops->close_device(device);
>> -	device->kvm = NULL;
>>  	if (iommufd)
>>  		vfio_iommufd_unbind(device);
>>  	else
>> @@ -465,6 +502,11 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>>  
>>  	vfio_device_group_close(device);
>>  
>> +	if (device->open_count == 0 && device->kvm) {
>> +		vfio_kvm_put_kvm(device->kvm);
>> +		device->kvm = NULL;
>> +	}
> 
> IIUC, device->open_count is protected by device->dev_set->lock.  Thanks,

Yep, thanks.  I will surround this bit of code with

mutex_lock(&device->dev_set->lock);
..
mutex_unlock(&device->dev_set->lock);
  

On Thu, Jan 12, 2023, Matthew Rosato wrote:
> On 1/12/23 4:05 PM, Alex Williamson wrote:
> > On Thu, 12 Jan 2023 15:38:44 -0500
> > Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> >> @@ -344,6 +345,35 @@ static bool vfio_assert_device_open(struct vfio_device *device)
> >>  	return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
> >>  }
> >>  
> >> +static bool vfio_kvm_get_kvm_safe(struct kvm *kvm)
> >> +{
> >> +	bool (*fn)(struct kvm *kvm);
> >> +	bool ret;
> >> +
> >> +	fn = symbol_get(kvm_get_kvm_safe);
> >> +	if (WARN_ON(!fn))

In a related vein to Alex's comments about error handling, this should not WARN.
WARNing during vfio_kvm_put_kvm() makes sense, but the "get" is somewhat blind.

> >> +		return false;
> >> +
> >> +	ret = fn(kvm);
> >> +
> >> +	symbol_put(kvm_get_kvm_safe);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static void vfio_kvm_put_kvm(struct kvm *kvm)
> >> +{
> >> +	void (*fn)(struct kvm *kvm);
> >> +
> >> +	fn = symbol_get(kvm_put_kvm);
> >> +	if (WARN_ON(!fn))
> >> +		return;
> >> +
> >> +	fn(kvm);
> >> +
> >> +	symbol_put(kvm_put_kvm);
> >> +}
  

On Thu, 12 Jan 2023 23:29:53 +0000
Sean Christopherson <seanjc@google.com> wrote:

> On Thu, Jan 12, 2023, Matthew Rosato wrote:
> > On 1/12/23 4:05 PM, Alex Williamson wrote:  
> > > On Thu, 12 Jan 2023 15:38:44 -0500
> > > Matthew Rosato <mjrosato@linux.ibm.com> wrote:  
> > >> @@ -344,6 +345,35 @@ static bool vfio_assert_device_open(struct vfio_device *device)
> > >>  	return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
> > >>  }
> > >>  
> > >> +static bool vfio_kvm_get_kvm_safe(struct kvm *kvm)
> > >> +{
> > >> +	bool (*fn)(struct kvm *kvm);
> > >> +	bool ret;
> > >> +
> > >> +	fn = symbol_get(kvm_get_kvm_safe);
> > >> +	if (WARN_ON(!fn))  
> 
> In a related vein to Alex's comments about error handling, this should not WARN.
> WARNing during vfio_kvm_put_kvm() makes sense, but the "get" is somewhat blind.

It's not exactly blind though, we wouldn't have a kvm pointer if the
kvm-vfio device hadn't stuffed one into the group.  We only call into
here if we have a non-NULL pointer, so it wouldn't simply be that the
kvm module isn't available for this to fire, but more that we have an
API change to make the symbol no longer exist.  A WARN for that doesn't
seem unreasonable.  Thanks,

Alex

> > >> +		return false;
> > >> +
> > >> +	ret = fn(kvm);
> > >> +
> > >> +	symbol_put(kvm_get_kvm_safe);
> > >> +
> > >> +	return ret;
> > >> +}
> > >> +
> > >> +static void vfio_kvm_put_kvm(struct kvm *kvm)
> > >> +{
> > >> +	void (*fn)(struct kvm *kvm);
> > >> +
> > >> +	fn = symbol_get(kvm_put_kvm);
> > >> +	if (WARN_ON(!fn))
> > >> +		return;
> > >> +
> > >> +	fn(kvm);
> > >> +
> > >> +	symbol_put(kvm_put_kvm);
> > >> +}  
>
  

On Thu, Jan 12, 2023, Alex Williamson wrote:
> On Thu, 12 Jan 2023 23:29:53 +0000
> Sean Christopherson <seanjc@google.com> wrote:
> 
> > On Thu, Jan 12, 2023, Matthew Rosato wrote:
> > > On 1/12/23 4:05 PM, Alex Williamson wrote:  
> > > > On Thu, 12 Jan 2023 15:38:44 -0500
> > > > Matthew Rosato <mjrosato@linux.ibm.com> wrote:  
> > > >> @@ -344,6 +345,35 @@ static bool vfio_assert_device_open(struct vfio_device *device)
> > > >>  	return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
> > > >>  }
> > > >>  
> > > >> +static bool vfio_kvm_get_kvm_safe(struct kvm *kvm)
> > > >> +{
> > > >> +	bool (*fn)(struct kvm *kvm);
> > > >> +	bool ret;
> > > >> +
> > > >> +	fn = symbol_get(kvm_get_kvm_safe);
> > > >> +	if (WARN_ON(!fn))  
> > 
> > In a related vein to Alex's comments about error handling, this should not WARN.
> > WARNing during vfio_kvm_put_kvm() makes sense, but the "get" is somewhat blind.
> 
> It's not exactly blind though, we wouldn't have a kvm pointer if the
> kvm-vfio device hadn't stuffed one into the group.  We only call into
> here if we have a non-NULL pointer, so it wouldn't simply be that the
> kvm module isn't available for this to fire, but more that we have an
> API change to make the symbol no longer exist.  A WARN for that doesn't
> seem unreasonable.  Thanks,

Hmm, I was thinking that it might be possible for kvm.ko to be on its way out,
but barring use of force module unload, which breaks things left and right, kvm.ko
can only be going if all VMs have been destroyed.

Agreed, WARN is fine.
  

Hi Matthew,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-tip/drm-tip]

url:    https://github.com/intel-lab-lkp/linux/commits/Matthew-Rosato/vfio-fix-potential-deadlock-on-vfio-group-lock/20230113-044050
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link:    https://lore.kernel.org/r/20230112203844.41179-1-mjrosato%40linux.ibm.com
patch subject: [Intel-gfx] [PATCH v2] vfio: fix potential deadlock on vfio group lock
config: arc-randconfig-r043-20230112
compiler: arc-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/efc312b2bb2125d4a447b166e323bf182e198901
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Matthew-Rosato/vfio-fix-potential-deadlock-on-vfio-group-lock/20230113-044050
        git checkout efc312b2bb2125d4a447b166e323bf182e198901
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/vfio/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/kvm_host.h:40,
                    from drivers/vfio/vfio_main.c:19:
>> include/uapi/linux/kvm.h:15:10: fatal error: asm/kvm.h: No such file or directory
      15 | #include <asm/kvm.h>
         |          ^~~~~~~~~~~
   compilation terminated.


vim +15 include/uapi/linux/kvm.h

6aa8b732ca01c3d include/linux/kvm.h      Avi Kivity             2006-12-10   4  
6aa8b732ca01c3d include/linux/kvm.h      Avi Kivity             2006-12-10   5  /*
6aa8b732ca01c3d include/linux/kvm.h      Avi Kivity             2006-12-10   6   * Userspace interface for /dev/kvm - kernel based virtual machine
6aa8b732ca01c3d include/linux/kvm.h      Avi Kivity             2006-12-10   7   *
dea8caee7b6971a include/linux/kvm.h      Rusty Russell          2007-07-17   8   * Note: you must update KVM_API_VERSION if you change this interface.
6aa8b732ca01c3d include/linux/kvm.h      Avi Kivity             2006-12-10   9   */
6aa8b732ca01c3d include/linux/kvm.h      Avi Kivity             2006-12-10  10  
fb1070d18edb37d include/uapi/linux/kvm.h Joe Richey             2021-05-21  11  #include <linux/const.h>
00bfddaf7f68a65 include/linux/kvm.h      Jaswinder Singh Rajput 2009-01-15  12  #include <linux/types.h>
97646202bc3f190 include/linux/kvm.h      Christian Borntraeger  2008-03-12  13  #include <linux/compiler.h>
6aa8b732ca01c3d include/linux/kvm.h      Avi Kivity             2006-12-10  14  #include <linux/ioctl.h>
f6a40e3bdf5fe0a include/linux/kvm.h      Jerone Young           2007-11-19 @15  #include <asm/kvm.h>
6aa8b732ca01c3d include/linux/kvm.h      Avi Kivity             2006-12-10  16
  

On 1/12/23 3:38 PM, Matthew Rosato 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:
> 
> kvm_put_kvm
>  -> kvm_destroy_vm
>   -> kvm_destroy_devices
>    -> kvm_vfio_destroy
>     -> kvm_vfio_file_set_kvm
>      -> vfio_file_set_kvm
>       -> group->group_lock/group_rwsem
> 
> Avoid this scenario by having vfio core code acquire a KVM reference
> the first time a device is opened and hold that reference until the
> device fd is closed, at a point after the group lock has been released.
> 
> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
> Changes from v1:
> * Re-write using symbol get logic to get kvm ref during first device
>   open, release the ref during device fd close after group lock is
>   released
> * Drop kvm get/put changes to drivers; now that vfio core holds a
>   kvm ref until sometime after the device_close op is called, it
>   should be fine for drivers to get and put their own references to it.
> ---
>  drivers/vfio/group.c     |  6 ++---
>  drivers/vfio/vfio_main.c | 48 +++++++++++++++++++++++++++++++++++++---
>  include/linux/vfio.h     |  1 -
>  3 files changed, 48 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> index bb24b2f0271e..2b0da82f82f4 100644
> --- a/drivers/vfio/group.c
> +++ b/drivers/vfio/group.c
> @@ -165,9 +165,9 @@ static int vfio_device_group_open(struct vfio_device *device)
>  	}
>  
>  	/*
> -	 * Here we pass the KVM pointer with the group under the lock.  If the
> -	 * device driver will use it, it must obtain a reference and release it
> -	 * during close_device.
> +	 * Here we pass the KVM pointer with the group under the lock.  A
> +	 * reference will be obtained the first time the device is opened and
> +	 * will be held until the device fd is closed.
>  	 */
>  	ret = vfio_device_open(device, device->group->iommufd,
>  			       device->group->kvm);
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 5177bb061b17..c969e2a0ecd3 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -16,6 +16,7 @@
>  #include <linux/fs.h>
>  #include <linux/idr.h>
>  #include <linux/iommu.h>
> +#include <linux/kvm_host.h>

Ugh, looks like including linux/kvm_host.h here breaks architectures that don't have an arch/*/include/uapi/asm/kvm.h

AFAICT this should be implicit with the CONFIG_HAVE_KVM bool, so unless someone has a better idea, to avoid I think we can key off of CONFIG_HAVE_KVM like so...

#ifdef CONFIG_HAVE_KVM
#include <linux/kvm_host.h>
#endif

[...]

#ifdef CONFIG_HAVE_KVM
[...symbol_get implementation here...]
#else
static bool vfio_kvm_get_kvm_safe(struct kvm *kvm)
{
	return false;
}
static void vfio_kvm_put_kvm(struct kvm *kvm)
{

}
#endif
  

On Fri, Jan 13, 2023 at 02:05:14AM +0000, Sean Christopherson wrote:
> On Thu, Jan 12, 2023, Alex Williamson wrote:
> > On Thu, 12 Jan 2023 23:29:53 +0000
> > Sean Christopherson <seanjc@google.com> wrote:
> > 
> > > On Thu, Jan 12, 2023, Matthew Rosato wrote:
> > > > On 1/12/23 4:05 PM, Alex Williamson wrote:  
> > > > > On Thu, 12 Jan 2023 15:38:44 -0500
> > > > > Matthew Rosato <mjrosato@linux.ibm.com> wrote:  
> > > > >> @@ -344,6 +345,35 @@ static bool vfio_assert_device_open(struct vfio_device *device)
> > > > >>  	return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
> > > > >>  }
> > > > >>  
> > > > >> +static bool vfio_kvm_get_kvm_safe(struct kvm *kvm)
> > > > >> +{
> > > > >> +	bool (*fn)(struct kvm *kvm);
> > > > >> +	bool ret;
> > > > >> +
> > > > >> +	fn = symbol_get(kvm_get_kvm_safe);
> > > > >> +	if (WARN_ON(!fn))  
> > > 
> > > In a related vein to Alex's comments about error handling, this should not WARN.
> > > WARNing during vfio_kvm_put_kvm() makes sense, but the "get" is somewhat blind.
> > 
> > It's not exactly blind though, we wouldn't have a kvm pointer if the
> > kvm-vfio device hadn't stuffed one into the group.  We only call into
> > here if we have a non-NULL pointer, so it wouldn't simply be that the
> > kvm module isn't available for this to fire, but more that we have an
> > API change to make the symbol no longer exist.  A WARN for that doesn't
> > seem unreasonable.  Thanks,
> 
> Hmm, I was thinking that it might be possible for kvm.ko to be on its way out,
> but barring use of force module unload, which breaks things left and right, kvm.ko
> can only be going if all VMs have been destroyed.

If we really care about these details then we should obtain both the
get_safe and put together, the put pointer should be stored in the
device and it should be symbol_put'd back once the kvm is put back and
it isn't needed any more.

This properly mimics how normal module stacking would work

Jason
  

On Thu, Jan 12, 2023 at 04:51:36PM -0500, Matthew Rosato wrote:

> Yep, thanks.  I will surround this bit of code with
> 
> mutex_lock(&device->dev_set->lock);
> ..
> mutex_unlock(&device->dev_set->lock);

Don't do it like that, copy the kvm out of the struct device to the
stack and NULL it. Then do the put without holding any locks.

Jason