| Message ID | 20230112203844.41179-1-mjrosato@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] vfio: fix potential deadlock on vfio group lock | expand |
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
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; 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; + } + 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 */
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(-)