| Message ID | 20230401144429.88673-7-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce new methods for verifying ownership in vfio PCI hot reset | expand |
Hi Yi, On 4/1/23 16:44, Yi Liu wrote: > This prepares vfio core to accept vfio device file from the vfio PCI > hot reset path. vfio_file_is_group() is still kept for KVM usage. > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Tested-by: Yanting Jiang <yanting.jiang@intel.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > --- > drivers/vfio/group.c | 32 ++++++++++++++------------------ > drivers/vfio/pci/vfio_pci_core.c | 4 ++-- > drivers/vfio/vfio.h | 2 ++ > drivers/vfio/vfio_main.c | 29 +++++++++++++++++++++++++++++ > include/linux/vfio.h | 1 + > 5 files changed, 48 insertions(+), 20 deletions(-) > > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c > index 27d5ba7cf9dc..d0c95d033605 100644 > --- a/drivers/vfio/group.c > +++ b/drivers/vfio/group.c > @@ -745,6 +745,15 @@ bool vfio_device_has_container(struct vfio_device *device) > return device->group->container; > } > > +struct vfio_group *vfio_group_from_file(struct file *file) > +{ > + struct vfio_group *group = file->private_data; > + > + if (file->f_op != &vfio_group_fops) > + return NULL; > + return group; > +} > + > /** > * vfio_file_iommu_group - Return the struct iommu_group for the vfio group file > * @file: VFIO group file > @@ -755,13 +764,13 @@ bool vfio_device_has_container(struct vfio_device *device) > */ > struct iommu_group *vfio_file_iommu_group(struct file *file) > { > - struct vfio_group *group = file->private_data; > + struct vfio_group *group = vfio_group_from_file(file); > struct iommu_group *iommu_group = NULL; > > if (!IS_ENABLED(CONFIG_SPAPR_TCE_IOMMU)) > return NULL; > > - if (!vfio_file_is_group(file)) > + if (!group) > return NULL; > > mutex_lock(&group->group_lock); > @@ -775,12 +784,12 @@ struct iommu_group *vfio_file_iommu_group(struct file *file) > EXPORT_SYMBOL_GPL(vfio_file_iommu_group); > > /** > - * vfio_file_is_group - True if the file is usable with VFIO aPIS > + * vfio_file_is_group - True if the file is a vfio group file > * @file: VFIO group file > */ > bool vfio_file_is_group(struct file *file) > { > - return file->f_op == &vfio_group_fops; > + return vfio_group_from_file(file); > } > EXPORT_SYMBOL_GPL(vfio_file_is_group); > > @@ -842,23 +851,10 @@ 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) > +bool vfio_group_has_dev(struct vfio_group *group, struct vfio_device *device) > { > - struct vfio_group *group = file->private_data; > - > - if (!vfio_file_is_group(file)) > - return false; > - > return group == device->group; > } > -EXPORT_SYMBOL_GPL(vfio_file_has_dev); > > static char *vfio_devnode(const struct device *dev, umode_t *mode) > { > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c > index b68fcba67a4b..2a510b71edcb 100644 > --- a/drivers/vfio/pci/vfio_pci_core.c > +++ b/drivers/vfio/pci/vfio_pci_core.c > @@ -1308,8 +1308,8 @@ vfio_pci_ioctl_pci_hot_reset_groups(struct vfio_pci_core_device *vdev, > break; > } > > - /* Ensure the FD is a vfio group FD.*/ > - if (!vfio_file_is_group(file)) { > + /* Ensure the FD is a vfio FD. vfio group or vfio device */ it is a bit strange to update the comment here and in the other places in this patch whereas file_is_valid still sticks to group file check By the way I would simply remove the comment which does not bring much > + if (!vfio_file_is_valid(file)) { > fput(file); > ret = -EINVAL; > break; > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h > index 7b19c621e0e6..c0aeea24fbd6 100644 > --- a/drivers/vfio/vfio.h > +++ b/drivers/vfio/vfio.h > @@ -84,6 +84,8 @@ void vfio_device_group_unregister(struct vfio_device *device); > int vfio_device_group_use_iommu(struct vfio_device *device); > void vfio_device_group_unuse_iommu(struct vfio_device *device); > void vfio_device_group_close(struct vfio_device *device); > +struct vfio_group *vfio_group_from_file(struct file *file); > +bool vfio_group_has_dev(struct vfio_group *group, struct vfio_device *device); > bool vfio_device_has_container(struct vfio_device *device); > int __init vfio_group_init(void); > void vfio_group_cleanup(void); > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > index 89497c933490..fe7446805afd 100644 > --- a/drivers/vfio/vfio_main.c > +++ b/drivers/vfio/vfio_main.c > @@ -1154,6 +1154,35 @@ const struct file_operations vfio_device_fops = { > .mmap = vfio_device_fops_mmap, > }; > > +/** > + * vfio_file_is_valid - True if the file is valid vfio file > + * @file: VFIO group file or VFIO device file I wonder if you shouldn't squash with next patch tbh. > + */ > +bool vfio_file_is_valid(struct file *file) > +{ > + return vfio_group_from_file(file); > +} > +EXPORT_SYMBOL_GPL(vfio_file_is_valid); > + > +/** > + * 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; > + > + group = vfio_group_from_file(file); > + if (!group) > + return false; > + > + return vfio_group_has_dev(group, device); > +} > +EXPORT_SYMBOL_GPL(vfio_file_has_dev); > + > /* > * Sub-module support > */ > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 97a1174b922f..f8fb9ab25188 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -258,6 +258,7 @@ int vfio_mig_get_next_state(struct vfio_device *device, > */ > struct iommu_group *vfio_file_iommu_group(struct file *file); > bool vfio_file_is_group(struct file *file); > +bool vfio_file_is_valid(struct file *file); > bool vfio_file_enforced_coherent(struct file *file); > void vfio_file_set_kvm(struct file *file, struct kvm *kvm); > bool vfio_file_has_dev(struct file *file, struct vfio_device *device); Eric
> From: Eric Auger <eric.auger@redhat.com> > Sent: Wednesday, April 5, 2023 4:28 PM > > Hi Yi, > On 4/1/23 16:44, Yi Liu wrote: > > This prepares vfio core to accept vfio device file from the vfio PCI > > hot reset path. vfio_file_is_group() is still kept for KVM usage. > > > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > > Tested-by: Yanting Jiang <yanting.jiang@intel.com> > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > --- > > drivers/vfio/group.c | 32 ++++++++++++++------------------ > > drivers/vfio/pci/vfio_pci_core.c | 4 ++-- > > drivers/vfio/vfio.h | 2 ++ > > drivers/vfio/vfio_main.c | 29 +++++++++++++++++++++++++++++ > > include/linux/vfio.h | 1 + > > 5 files changed, 48 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c > > index 27d5ba7cf9dc..d0c95d033605 100644 > > --- a/drivers/vfio/group.c > > +++ b/drivers/vfio/group.c > > @@ -745,6 +745,15 @@ bool vfio_device_has_container(struct vfio_device *device) > > return device->group->container; > > } > > > > +struct vfio_group *vfio_group_from_file(struct file *file) > > +{ > > + struct vfio_group *group = file->private_data; > > + > > + if (file->f_op != &vfio_group_fops) > > + return NULL; > > + return group; > > +} > > + > > /** > > * vfio_file_iommu_group - Return the struct iommu_group for the vfio group file > > * @file: VFIO group file > > @@ -755,13 +764,13 @@ bool vfio_device_has_container(struct vfio_device > *device) > > */ > > struct iommu_group *vfio_file_iommu_group(struct file *file) > > { > > - struct vfio_group *group = file->private_data; > > + struct vfio_group *group = vfio_group_from_file(file); > > struct iommu_group *iommu_group = NULL; > > > > if (!IS_ENABLED(CONFIG_SPAPR_TCE_IOMMU)) > > return NULL; > > > > - if (!vfio_file_is_group(file)) > > + if (!group) > > return NULL; > > > > mutex_lock(&group->group_lock); > > @@ -775,12 +784,12 @@ struct iommu_group *vfio_file_iommu_group(struct file > *file) > > EXPORT_SYMBOL_GPL(vfio_file_iommu_group); > > > > /** > > - * vfio_file_is_group - True if the file is usable with VFIO aPIS > > + * vfio_file_is_group - True if the file is a vfio group file > > * @file: VFIO group file > > */ > > bool vfio_file_is_group(struct file *file) > > { > > - return file->f_op == &vfio_group_fops; > > + return vfio_group_from_file(file); > > } > > EXPORT_SYMBOL_GPL(vfio_file_is_group); > > > > @@ -842,23 +851,10 @@ 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) > > +bool vfio_group_has_dev(struct vfio_group *group, struct vfio_device *device) > > { > > - struct vfio_group *group = file->private_data; > > - > > - if (!vfio_file_is_group(file)) > > - return false; > > - > > return group == device->group; > > } > > -EXPORT_SYMBOL_GPL(vfio_file_has_dev); > > > > static char *vfio_devnode(const struct device *dev, umode_t *mode) > > { > > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c > > index b68fcba67a4b..2a510b71edcb 100644 > > --- a/drivers/vfio/pci/vfio_pci_core.c > > +++ b/drivers/vfio/pci/vfio_pci_core.c > > @@ -1308,8 +1308,8 @@ vfio_pci_ioctl_pci_hot_reset_groups(struct > vfio_pci_core_device *vdev, > > break; > > } > > > > - /* Ensure the FD is a vfio group FD.*/ > > - if (!vfio_file_is_group(file)) { > > + /* Ensure the FD is a vfio FD. vfio group or vfio device */ > it is a bit strange to update the comment here and in the other places > in this patch whereas file_is_valid still sticks to group file check > By the way I would simply remove the comment which does not bring much ok. yeah, at this moment, it's still group file. may just delete this comment. > > + if (!vfio_file_is_valid(file)) { > > fput(file); > > ret = -EINVAL; > > break; > > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h > > index 7b19c621e0e6..c0aeea24fbd6 100644 > > --- a/drivers/vfio/vfio.h > > +++ b/drivers/vfio/vfio.h > > @@ -84,6 +84,8 @@ void vfio_device_group_unregister(struct vfio_device *device); > > int vfio_device_group_use_iommu(struct vfio_device *device); > > void vfio_device_group_unuse_iommu(struct vfio_device *device); > > void vfio_device_group_close(struct vfio_device *device); > > +struct vfio_group *vfio_group_from_file(struct file *file); > > +bool vfio_group_has_dev(struct vfio_group *group, struct vfio_device *device); > > bool vfio_device_has_container(struct vfio_device *device); > > int __init vfio_group_init(void); > > void vfio_group_cleanup(void); > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > > index 89497c933490..fe7446805afd 100644 > > --- a/drivers/vfio/vfio_main.c > > +++ b/drivers/vfio/vfio_main.c > > @@ -1154,6 +1154,35 @@ const struct file_operations vfio_device_fops = { > > .mmap = vfio_device_fops_mmap, > > }; > > > > +/** > > + * vfio_file_is_valid - True if the file is valid vfio file > > + * @file: VFIO group file or VFIO device file > I wonder if you shouldn't squash with next patch tbh. yes. this is still group file, no device file yet. Thanks, Yi Liu > > + */ > > +bool vfio_file_is_valid(struct file *file) > > +{ > > + return vfio_group_from_file(file); > > +} > > +EXPORT_SYMBOL_GPL(vfio_file_is_valid); > > + > > +/** > > + * 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; > > + > > + group = vfio_group_from_file(file); > > + if (!group) > > + return false; > > + > > + return vfio_group_has_dev(group, device); > > +} > > +EXPORT_SYMBOL_GPL(vfio_file_has_dev); > > + > > /* > > * Sub-module support > > */ > > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > > index 97a1174b922f..f8fb9ab25188 100644 > > --- a/include/linux/vfio.h > > +++ b/include/linux/vfio.h > > @@ -258,6 +258,7 @@ int vfio_mig_get_next_state(struct vfio_device *device, > > */ > > struct iommu_group *vfio_file_iommu_group(struct file *file); > > bool vfio_file_is_group(struct file *file); > > +bool vfio_file_is_valid(struct file *file); > > bool vfio_file_enforced_coherent(struct file *file); > > void vfio_file_set_kvm(struct file *file, struct kvm *kvm); > > bool vfio_file_has_dev(struct file *file, struct vfio_device *device); > Eric
diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c index 27d5ba7cf9dc..d0c95d033605 100644 --- a/drivers/vfio/group.c +++ b/drivers/vfio/group.c @@ -745,6 +745,15 @@ bool vfio_device_has_container(struct vfio_device *device) return device->group->container; } +struct vfio_group *vfio_group_from_file(struct file *file) +{ + struct vfio_group *group = file->private_data; + + if (file->f_op != &vfio_group_fops) + return NULL; + return group; +} + /** * vfio_file_iommu_group - Return the struct iommu_group for the vfio group file * @file: VFIO group file @@ -755,13 +764,13 @@ bool vfio_device_has_container(struct vfio_device *device) */ struct iommu_group *vfio_file_iommu_group(struct file *file) { - struct vfio_group *group = file->private_data; + struct vfio_group *group = vfio_group_from_file(file); struct iommu_group *iommu_group = NULL; if (!IS_ENABLED(CONFIG_SPAPR_TCE_IOMMU)) return NULL; - if (!vfio_file_is_group(file)) + if (!group) return NULL; mutex_lock(&group->group_lock); @@ -775,12 +784,12 @@ struct iommu_group *vfio_file_iommu_group(struct file *file) EXPORT_SYMBOL_GPL(vfio_file_iommu_group); /** - * vfio_file_is_group - True if the file is usable with VFIO aPIS + * vfio_file_is_group - True if the file is a vfio group file * @file: VFIO group file */ bool vfio_file_is_group(struct file *file) { - return file->f_op == &vfio_group_fops; + return vfio_group_from_file(file); } EXPORT_SYMBOL_GPL(vfio_file_is_group); @@ -842,23 +851,10 @@ 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) +bool vfio_group_has_dev(struct vfio_group *group, struct vfio_device *device) { - struct vfio_group *group = file->private_data; - - if (!vfio_file_is_group(file)) - return false; - return group == device->group; } -EXPORT_SYMBOL_GPL(vfio_file_has_dev); static char *vfio_devnode(const struct device *dev, umode_t *mode) { diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index b68fcba67a4b..2a510b71edcb 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -1308,8 +1308,8 @@ vfio_pci_ioctl_pci_hot_reset_groups(struct vfio_pci_core_device *vdev, break; } - /* Ensure the FD is a vfio group FD.*/ - if (!vfio_file_is_group(file)) { + /* Ensure the FD is a vfio FD. vfio group or vfio device */ + if (!vfio_file_is_valid(file)) { fput(file); ret = -EINVAL; break; diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h index 7b19c621e0e6..c0aeea24fbd6 100644 --- a/drivers/vfio/vfio.h +++ b/drivers/vfio/vfio.h @@ -84,6 +84,8 @@ void vfio_device_group_unregister(struct vfio_device *device); int vfio_device_group_use_iommu(struct vfio_device *device); void vfio_device_group_unuse_iommu(struct vfio_device *device); void vfio_device_group_close(struct vfio_device *device); +struct vfio_group *vfio_group_from_file(struct file *file); +bool vfio_group_has_dev(struct vfio_group *group, struct vfio_device *device); bool vfio_device_has_container(struct vfio_device *device); int __init vfio_group_init(void); void vfio_group_cleanup(void); diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index 89497c933490..fe7446805afd 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -1154,6 +1154,35 @@ const struct file_operations vfio_device_fops = { .mmap = vfio_device_fops_mmap, }; +/** + * vfio_file_is_valid - True if the file is valid vfio file + * @file: VFIO group file or VFIO device file + */ +bool vfio_file_is_valid(struct file *file) +{ + return vfio_group_from_file(file); +} +EXPORT_SYMBOL_GPL(vfio_file_is_valid); + +/** + * 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; + + group = vfio_group_from_file(file); + if (!group) + return false; + + return vfio_group_has_dev(group, device); +} +EXPORT_SYMBOL_GPL(vfio_file_has_dev); + /* * Sub-module support */ diff --git a/include/linux/vfio.h b/include/linux/vfio.h index 97a1174b922f..f8fb9ab25188 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -258,6 +258,7 @@ int vfio_mig_get_next_state(struct vfio_device *device, */ struct iommu_group *vfio_file_iommu_group(struct file *file); bool vfio_file_is_group(struct file *file); +bool vfio_file_is_valid(struct file *file); bool vfio_file_enforced_coherent(struct file *file); void vfio_file_set_kvm(struct file *file, struct kvm *kvm); bool vfio_file_has_dev(struct file *file, struct vfio_device *device);