| Message ID | 8-v2-6a528653a750+1578a-vfio_kvm_no_group_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove vfio_group from the struct file facing VFIO API | expand |
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Thursday, April 21, 2022 3:23 AM > > VFIO PCI does a security check as part of hot reset to prove that the user > has permission to manipulate all the devices that will be impacted by the > reset. > > Use a new API vfio_file_has_dev() to perform this security check against > the struct file directly and remove the vfio_group from VFIO PCI. > > Since VFIO PCI was the last user of vfio_group_get_external_user() remove > it as well. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > drivers/vfio/pci/vfio_pci_core.c | 42 ++++++++++----------- > drivers/vfio/vfio.c | 63 +++++++++----------------------- > include/linux/vfio.h | 2 +- > 3 files changed, 40 insertions(+), 67 deletions(-) > > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c > index b7bb16f92ac628..465c42f53fd2fc 100644 > --- a/drivers/vfio/pci/vfio_pci_core.c > +++ b/drivers/vfio/pci/vfio_pci_core.c > @@ -577,7 +577,7 @@ static int vfio_pci_fill_devs(struct pci_dev *pdev, void > *data) > > struct vfio_pci_group_info { > int count; > - struct vfio_group **groups; > + struct file **files; > }; > > static bool vfio_pci_dev_below_slot(struct pci_dev *pdev, struct pci_slot > *slot) > @@ -1039,10 +1039,10 @@ long vfio_pci_core_ioctl(struct vfio_device > *core_vdev, unsigned int cmd, > } else if (cmd == VFIO_DEVICE_PCI_HOT_RESET) { > struct vfio_pci_hot_reset hdr; > int32_t *group_fds; > - struct vfio_group **groups; > + struct file **files; > struct vfio_pci_group_info info; > bool slot = false; > - int group_idx, count = 0, ret = 0; > + int file_idx, count = 0, ret = 0; > > minsz = offsetofend(struct vfio_pci_hot_reset, count); > > @@ -1075,17 +1075,17 @@ long vfio_pci_core_ioctl(struct vfio_device > *core_vdev, unsigned int cmd, > return -EINVAL; > > group_fds = kcalloc(hdr.count, sizeof(*group_fds), > GFP_KERNEL); > - groups = kcalloc(hdr.count, sizeof(*groups), GFP_KERNEL); > - if (!group_fds || !groups) { > + files = kcalloc(hdr.count, sizeof(*files), GFP_KERNEL); > + if (!group_fds || !files) { > kfree(group_fds); > - kfree(groups); > + kfree(files); > return -ENOMEM; > } > > if (copy_from_user(group_fds, (void __user *)(arg + minsz), > hdr.count * sizeof(*group_fds))) { > kfree(group_fds); > - kfree(groups); > + kfree(files); > return -EFAULT; > } > > @@ -1094,22 +1094,22 @@ long vfio_pci_core_ioctl(struct vfio_device > *core_vdev, unsigned int cmd, > * user interface and store the group and iommu ID. This > * ensures the group is held across the reset. > */ > - for (group_idx = 0; group_idx < hdr.count; group_idx++) { > - struct vfio_group *group; > - struct fd f = fdget(group_fds[group_idx]); > - if (!f.file) { > + for (file_idx = 0; file_idx < hdr.count; file_idx++) { > + struct file *file = fget(group_fds[file_idx]); > + > + if (!file) { > ret = -EBADF; > break; > } > > - group = vfio_group_get_external_user(f.file); > - fdput(f); > - if (IS_ERR(group)) { > - ret = PTR_ERR(group); > + /* Ensure the FD is a vfio group FD.*/ > + if (!vfio_file_iommu_group(file)) { > + fput(file); > + ret = -EINVAL; > break; > } > > - groups[group_idx] = group; > + files[file_idx] = file; > } > > kfree(group_fds); > @@ -1119,15 +1119,15 @@ long vfio_pci_core_ioctl(struct vfio_device > *core_vdev, unsigned int cmd, > goto hot_reset_release; > > info.count = hdr.count; > - info.groups = groups; > + info.files = files; > > ret = vfio_pci_dev_set_hot_reset(vdev->vdev.dev_set, &info); > > hot_reset_release: > - for (group_idx--; group_idx >= 0; group_idx--) > - vfio_group_put_external_user(groups[group_idx]); > + for (file_idx--; file_idx >= 0; file_idx--) > + fput(files[file_idx]); > > - kfree(groups); > + kfree(files); > return ret; > } else if (cmd == VFIO_DEVICE_IOEVENTFD) { > struct vfio_device_ioeventfd ioeventfd; > @@ -1964,7 +1964,7 @@ static bool vfio_dev_in_groups(struct > vfio_pci_core_device *vdev, > unsigned int i; > > for (i = 0; i < groups->count; i++) > - if (groups->groups[i] == vdev->vdev.group) > + if (vfio_file_has_dev(groups->files[i], &vdev->vdev)) > return true; > return false; > } > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index 7d0fad02936f69..ff5f6e0f285faa 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -1899,51 +1899,6 @@ static const struct file_operations > vfio_device_fops = { > .mmap = vfio_device_fops_mmap, > }; > > -/* > - * External user API, exported by symbols to be linked dynamically. > - * > - * The protocol includes: > - * 1. do normal VFIO init operation: > - * - opening a new container; > - * - attaching group(s) to it; > - * - setting an IOMMU driver for a container. > - * When IOMMU is set for a container, all groups in it are > - * considered ready to use by an external user. > - * > - * 2. User space passes a group fd to an external user. > - * The external user calls vfio_group_get_external_user() > - * to verify that: > - * - the group is initialized; > - * - IOMMU is set for it. > - * If both checks passed, vfio_group_get_external_user() > - * increments the container user counter to prevent > - * the VFIO group from disposal before KVM exits. > - * > - * 3. When the external KVM finishes, it calls > - * vfio_group_put_external_user() to release the VFIO group. > - * This call decrements the container user counter. > - */ > -struct vfio_group *vfio_group_get_external_user(struct file *filep) > -{ > - struct vfio_group *group = filep->private_data; > - int ret; > - > - if (filep->f_op != &vfio_group_fops) > - return ERR_PTR(-EINVAL); > - > - ret = vfio_group_add_container_user(group); > - if (ret) > - return ERR_PTR(ret); > - > - /* > - * Since the caller holds the fget on the file group->users must be >= 1 > - */ > - vfio_group_get(group); > - > - return group; > -} > -EXPORT_SYMBOL_GPL(vfio_group_get_external_user); > - > /* > * External user API, exported by symbols to be linked dynamically. > * The external user passes in a device pointer > @@ -2056,6 +2011,24 @@ void vfio_file_set_kvm(struct file *file, struct kvm > *kvm) > } > EXPORT_SYMBOL_GPL(vfio_file_set_kvm); > > +/** > + * vfio_file_has_dev - True if the VFIO file is a handle for device > + * @file: VFIO file to check > + * @device: Device that must be part of the file > + * > + * Returns true if given file has permission to manipulate the given device. > + */ > +bool vfio_file_has_dev(struct file *file, struct vfio_device *device) > +{ > + struct vfio_group *group = file->private_data; > + > + if (file->f_op != &vfio_group_fops) > + return false; > + > + return group == device->group; > +} > +EXPORT_SYMBOL_GPL(vfio_file_has_dev); > + > /* > * Sub-module support > */ > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index cbd9103b5c1223..e8be8ec40f2b50 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -140,13 +140,13 @@ int vfio_mig_get_next_state(struct vfio_device > *device, > /* > * External user API > */ > -extern struct vfio_group *vfio_group_get_external_user(struct file *filep); > extern void vfio_group_put_external_user(struct vfio_group *group); > extern struct vfio_group *vfio_group_get_external_user_from_dev(struct > device > *dev); > extern struct iommu_group *vfio_file_iommu_group(struct file *file); > extern bool vfio_file_enforced_coherent(struct file *file); > extern void vfio_file_set_kvm(struct file *file, struct kvm *kvm); > +extern bool vfio_file_has_dev(struct file *file, struct vfio_device *device); > > #define VFIO_PIN_PAGES_MAX_ENTRIES (PAGE_SIZE/sizeof(unsigned > long)) > > -- > 2.36.0
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index b7bb16f92ac628..465c42f53fd2fc 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -577,7 +577,7 @@ static int vfio_pci_fill_devs(struct pci_dev *pdev, void *data) struct vfio_pci_group_info { int count; - struct vfio_group **groups; + struct file **files; }; static bool vfio_pci_dev_below_slot(struct pci_dev *pdev, struct pci_slot *slot) @@ -1039,10 +1039,10 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd, } else if (cmd == VFIO_DEVICE_PCI_HOT_RESET) { struct vfio_pci_hot_reset hdr; int32_t *group_fds; - struct vfio_group **groups; + struct file **files; struct vfio_pci_group_info info; bool slot = false; - int group_idx, count = 0, ret = 0; + int file_idx, count = 0, ret = 0; minsz = offsetofend(struct vfio_pci_hot_reset, count); @@ -1075,17 +1075,17 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd, return -EINVAL; group_fds = kcalloc(hdr.count, sizeof(*group_fds), GFP_KERNEL); - groups = kcalloc(hdr.count, sizeof(*groups), GFP_KERNEL); - if (!group_fds || !groups) { + files = kcalloc(hdr.count, sizeof(*files), GFP_KERNEL); + if (!group_fds || !files) { kfree(group_fds); - kfree(groups); + kfree(files); return -ENOMEM; } if (copy_from_user(group_fds, (void __user *)(arg + minsz), hdr.count * sizeof(*group_fds))) { kfree(group_fds); - kfree(groups); + kfree(files); return -EFAULT; } @@ -1094,22 +1094,22 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd, * user interface and store the group and iommu ID. This * ensures the group is held across the reset. */ - for (group_idx = 0; group_idx < hdr.count; group_idx++) { - struct vfio_group *group; - struct fd f = fdget(group_fds[group_idx]); - if (!f.file) { + for (file_idx = 0; file_idx < hdr.count; file_idx++) { + struct file *file = fget(group_fds[file_idx]); + + if (!file) { ret = -EBADF; break; } - group = vfio_group_get_external_user(f.file); - fdput(f); - if (IS_ERR(group)) { - ret = PTR_ERR(group); + /* Ensure the FD is a vfio group FD.*/ + if (!vfio_file_iommu_group(file)) { + fput(file); + ret = -EINVAL; break; } - groups[group_idx] = group; + files[file_idx] = file; } kfree(group_fds); @@ -1119,15 +1119,15 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd, goto hot_reset_release; info.count = hdr.count; - info.groups = groups; + info.files = files; ret = vfio_pci_dev_set_hot_reset(vdev->vdev.dev_set, &info); hot_reset_release: - for (group_idx--; group_idx >= 0; group_idx--) - vfio_group_put_external_user(groups[group_idx]); + for (file_idx--; file_idx >= 0; file_idx--) + fput(files[file_idx]); - kfree(groups); + kfree(files); return ret; } else if (cmd == VFIO_DEVICE_IOEVENTFD) { struct vfio_device_ioeventfd ioeventfd; @@ -1964,7 +1964,7 @@ static bool vfio_dev_in_groups(struct vfio_pci_core_device *vdev, unsigned int i; for (i = 0; i < groups->count; i++) - if (groups->groups[i] == vdev->vdev.group) + if (vfio_file_has_dev(groups->files[i], &vdev->vdev)) return true; return false; } diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index 7d0fad02936f69..ff5f6e0f285faa 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -1899,51 +1899,6 @@ static const struct file_operations vfio_device_fops = { .mmap = vfio_device_fops_mmap, }; -/* - * External user API, exported by symbols to be linked dynamically. - * - * The protocol includes: - * 1. do normal VFIO init operation: - * - opening a new container; - * - attaching group(s) to it; - * - setting an IOMMU driver for a container. - * When IOMMU is set for a container, all groups in it are - * considered ready to use by an external user. - * - * 2. User space passes a group fd to an external user. - * The external user calls vfio_group_get_external_user() - * to verify that: - * - the group is initialized; - * - IOMMU is set for it. - * If both checks passed, vfio_group_get_external_user() - * increments the container user counter to prevent - * the VFIO group from disposal before KVM exits. - * - * 3. When the external KVM finishes, it calls - * vfio_group_put_external_user() to release the VFIO group. - * This call decrements the container user counter. - */ -struct vfio_group *vfio_group_get_external_user(struct file *filep) -{ - struct vfio_group *group = filep->private_data; - int ret; - - if (filep->f_op != &vfio_group_fops) - return ERR_PTR(-EINVAL); - - ret = vfio_group_add_container_user(group); - if (ret) - return ERR_PTR(ret); - - /* - * Since the caller holds the fget on the file group->users must be >= 1 - */ - vfio_group_get(group); - - return group; -} -EXPORT_SYMBOL_GPL(vfio_group_get_external_user); - /* * External user API, exported by symbols to be linked dynamically. * The external user passes in a device pointer @@ -2056,6 +2011,24 @@ void vfio_file_set_kvm(struct file *file, struct kvm *kvm) } EXPORT_SYMBOL_GPL(vfio_file_set_kvm); +/** + * vfio_file_has_dev - True if the VFIO file is a handle for device + * @file: VFIO file to check + * @device: Device that must be part of the file + * + * Returns true if given file has permission to manipulate the given device. + */ +bool vfio_file_has_dev(struct file *file, struct vfio_device *device) +{ + struct vfio_group *group = file->private_data; + + if (file->f_op != &vfio_group_fops) + return false; + + return group == device->group; +} +EXPORT_SYMBOL_GPL(vfio_file_has_dev); + /* * Sub-module support */ diff --git a/include/linux/vfio.h b/include/linux/vfio.h index cbd9103b5c1223..e8be8ec40f2b50 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -140,13 +140,13 @@ int vfio_mig_get_next_state(struct vfio_device *device, /* * External user API */ -extern struct vfio_group *vfio_group_get_external_user(struct file *filep); extern void vfio_group_put_external_user(struct vfio_group *group); extern struct vfio_group *vfio_group_get_external_user_from_dev(struct device *dev); extern struct iommu_group *vfio_file_iommu_group(struct file *file); extern bool vfio_file_enforced_coherent(struct file *file); extern void vfio_file_set_kvm(struct file *file, struct kvm *kvm); +extern bool vfio_file_has_dev(struct file *file, struct vfio_device *device); #define VFIO_PIN_PAGES_MAX_ENTRIES (PAGE_SIZE/sizeof(unsigned long))
VFIO PCI does a security check as part of hot reset to prove that the user has permission to manipulate all the devices that will be impacted by the reset. Use a new API vfio_file_has_dev() to perform this security check against the struct file directly and remove the vfio_group from VFIO PCI. Since VFIO PCI was the last user of vfio_group_get_external_user() remove it as well. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/vfio/pci/vfio_pci_core.c | 42 ++++++++++----------- drivers/vfio/vfio.c | 63 +++++++++----------------------- include/linux/vfio.h | 2 +- 3 files changed, 40 insertions(+), 67 deletions(-)