| Message ID | 20210811151500.2744-6-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/14] vfio: Move vfio_iommu_group_get() to vfio_register_group_dev() | expand |
On Wed, 11 Aug 2021 17:14:51 +0200 Christoph Hellwig <hch@lst.de> wrote: > @@ -833,14 +789,61 @@ void vfio_uninit_group_dev(struct vfio_device *device) > } > EXPORT_SYMBOL_GPL(vfio_uninit_group_dev); > > -struct vfio_group *vfio_group_find_or_alloc(struct device *dev) > +#ifdef CONFIG_VFIO_NOIOMMU > +static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev) > { > struct iommu_group *iommu_group; > struct vfio_group *group; > + int ret; > > - iommu_group = vfio_iommu_group_get(dev); > - if (!iommu_group) > + iommu_group = iommu_group_alloc(); > + if (IS_ERR(iommu_group)) > + return ERR_CAST(iommu_group); > + > + iommu_group_set_name(iommu_group, "vfio-noiommu"); > + iommu_group_set_iommudata(iommu_group, &noiommu, NULL); > + ret = iommu_group_add_device(iommu_group, dev); > + if (ret) > + goto out_put_group; > + > + group = vfio_create_group(iommu_group); > + if (IS_ERR(group)) { > + ret = PTR_ERR(group); > + goto out_remove_device; > + } > + > + return group; > + > +out_remove_device: > + iommu_group_remove_device(dev); > +out_put_group: > + iommu_group_put(iommu_group); > + return ERR_PTR(ret); > +} > +#endif > + > +static struct vfio_group *vfio_group_find_or_alloc(struct device *dev) > +{ > + struct iommu_group *iommu_group; > + struct vfio_group *group; > + > + iommu_group = iommu_group_get(dev); > + if (!iommu_group) { > +#ifdef CONFIG_VFIO_NOIOMMU > + /* > + * With noiommu enabled, create an IOMMU group for devices that > + * don't already have one and don't have an iommu_ops on their > + * bus. Taint the kernel because we're about to give a DMA > + * capable device to a user without IOMMU protection. > + */ > + if (noiommu && !iommu_present(dev->bus)) { > + add_taint(TAINT_USER, LOCKDEP_STILL_OK); > + dev_warn(dev, "Adding kernel taint for vfio-noiommu group on device\n"); > + return vfio_noiommu_group_alloc(dev); Nit, we taint regardless of the success of this function, should we move the tainting back into the function (using the flags to skip for mdev in subsequent patches) or swap the order to check the return value before tainting? Thanks, Alex
On Wed, Aug 11, 2021 at 04:03:41PM -0600, Alex Williamson wrote: > > + dev_warn(dev, "Adding kernel taint for vfio-noiommu group on device\n"); > > + return vfio_noiommu_group_alloc(dev); > > Nit, we taint regardless of the success of this function, should we > move the tainting back into the function (using the flags to skip for > mdev in subsequent patches) or swap the order to check the return value > before tainting? Thanks, Does it really matter to have the extra thread if a memory allocation failed when going down this route?
On Thu, 12 Aug 2021 09:26:17 +0200 Christoph Hellwig <hch@lst.de> wrote: > On Wed, Aug 11, 2021 at 04:03:41PM -0600, Alex Williamson wrote: > > > + dev_warn(dev, "Adding kernel taint for vfio-noiommu group on device\n"); > > > + return vfio_noiommu_group_alloc(dev); > > > > Nit, we taint regardless of the success of this function, should we > > move the tainting back into the function (using the flags to skip for > > mdev in subsequent patches) or swap the order to check the return value > > before tainting? Thanks, > > Does it really matter to have the extra thread if a memory allocation > failed when going down this route? Extra thread? In practice this is unlikely to ever fail, but if we've chosen the point at which we have a no-iommu group as where we taint, then let's at least be consistent and not move that back to the point where we tried to make a no-iommu group, regardless of whether it was successful. Thanks, Alex
On Thu, Aug 12, 2021 at 09:56:14AM -0600, Alex Williamson wrote: > On Thu, 12 Aug 2021 09:26:17 +0200 > Christoph Hellwig <hch@lst.de> wrote: > > > On Wed, Aug 11, 2021 at 04:03:41PM -0600, Alex Williamson wrote: > > > > + dev_warn(dev, "Adding kernel taint for vfio-noiommu group on device\n"); > > > > + return vfio_noiommu_group_alloc(dev); > > > > > > Nit, we taint regardless of the success of this function, should we > > > move the tainting back into the function (using the flags to skip for > > > mdev in subsequent patches) or swap the order to check the return value > > > before tainting? Thanks, > > > > Does it really matter to have the extra thread if a memory allocation > > failed when going down this route? > > Extra thread? In practice this is unlikely to ever fail, but if we've > chosen the point at which we have a no-iommu group as where we taint, > then let's at least be consistent and not move that back to the point > where we tried to make a no-iommu group, regardless of whether it was > successful. Thanks, Sorry, the mental spell checker kicked in. Thread should have read taint instead. But if you don't want to tain in the failure case I'll need to refactor this a bit.
Here is a version with the tain only on successful groip allocation: http://git.infradead.org/users/hch/misc.git/commit/bdb5d2401ebd43ae6c069aeaa8a64e0c774dd104 I'm not going to spam the list with the whole series until a few more reviews are in.
On Sun, Aug 15, 2021 at 05:48:34PM +0200, Christoph Hellwig wrote: > Here is a version with the tain only on successful groip allocation: > > http://git.infradead.org/users/hch/misc.git/commit/bdb5d2401ebd43ae6c069aeaa8a64e0c774dd104 > > I'm not going to spam the list with the whole series until a few more > reviews are in. I hope to look at it next week after I unbury my mailboxes.. Thanks, Jason
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index 9e97ad36a1c052..d96acd7af50398 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -169,50 +169,6 @@ static void vfio_release_device_set(struct vfio_device *device) xa_unlock(&vfio_device_set_xa); } -static struct iommu_group *vfio_iommu_group_get(struct device *dev) -{ - struct iommu_group *group; - int __maybe_unused ret; - - group = iommu_group_get(dev); - -#ifdef CONFIG_VFIO_NOIOMMU - /* - * With noiommu enabled, an IOMMU group will be created for a device - * that doesn't already have one and doesn't have an iommu_ops on their - * bus. We set iommudata simply to be able to identify these groups - * as special use and for reclamation later. - */ - if (group || !noiommu || iommu_present(dev->bus)) - return group; - - group = iommu_group_alloc(); - if (IS_ERR(group)) - return NULL; - - iommu_group_set_name(group, "vfio-noiommu"); - iommu_group_set_iommudata(group, &noiommu, NULL); - ret = iommu_group_add_device(group, dev); - if (ret) { - iommu_group_put(group); - return NULL; - } - - /* - * Where to taint? At this point we've added an IOMMU group for a - * device that is not backed by iommu_ops, therefore any iommu_ - * callback using iommu_ops can legitimately Oops. So, while we may - * be about to give a DMA capable device to a user without IOMMU - * protection, which is clearly taint-worthy, let's go ahead and do - * it here. - */ - add_taint(TAINT_USER, LOCKDEP_STILL_OK); - dev_warn(dev, "Adding kernel taint for vfio-noiommu group on device\n"); -#endif - - return group; -} - static void vfio_iommu_group_put(struct iommu_group *group, struct device *dev) { #ifdef CONFIG_VFIO_NOIOMMU @@ -833,14 +789,61 @@ void vfio_uninit_group_dev(struct vfio_device *device) } EXPORT_SYMBOL_GPL(vfio_uninit_group_dev); -struct vfio_group *vfio_group_find_or_alloc(struct device *dev) +#ifdef CONFIG_VFIO_NOIOMMU +static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev) { struct iommu_group *iommu_group; struct vfio_group *group; + int ret; - iommu_group = vfio_iommu_group_get(dev); - if (!iommu_group) + iommu_group = iommu_group_alloc(); + if (IS_ERR(iommu_group)) + return ERR_CAST(iommu_group); + + iommu_group_set_name(iommu_group, "vfio-noiommu"); + iommu_group_set_iommudata(iommu_group, &noiommu, NULL); + ret = iommu_group_add_device(iommu_group, dev); + if (ret) + goto out_put_group; + + group = vfio_create_group(iommu_group); + if (IS_ERR(group)) { + ret = PTR_ERR(group); + goto out_remove_device; + } + + return group; + +out_remove_device: + iommu_group_remove_device(dev); +out_put_group: + iommu_group_put(iommu_group); + return ERR_PTR(ret); +} +#endif + +static struct vfio_group *vfio_group_find_or_alloc(struct device *dev) +{ + struct iommu_group *iommu_group; + struct vfio_group *group; + + iommu_group = iommu_group_get(dev); + if (!iommu_group) { +#ifdef CONFIG_VFIO_NOIOMMU + /* + * With noiommu enabled, create an IOMMU group for devices that + * don't already have one and don't have an iommu_ops on their + * bus. Taint the kernel because we're about to give a DMA + * capable device to a user without IOMMU protection. + */ + if (noiommu && !iommu_present(dev->bus)) { + add_taint(TAINT_USER, LOCKDEP_STILL_OK); + dev_warn(dev, "Adding kernel taint for vfio-noiommu group on device\n"); + return vfio_noiommu_group_alloc(dev); + } +#endif return ERR_PTR(-EINVAL); + } /* * A found vfio_group already holds a reference to the iommu_group. @@ -852,7 +855,8 @@ struct vfio_group *vfio_group_find_or_alloc(struct device *dev) if (!IS_ERR(group)) return group; } - vfio_iommu_group_put(iommu_group, dev); + + iommu_group_put(iommu_group); return group; }
Split the actual noiommu group creation from vfio_iommu_group_get into a new helper, and open code the rest of vfio_iommu_group_get in its only caller. This creates an antirely separate and clear code path for the noiommu group creation. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/vfio/vfio.c | 100 +++++++++++++++++++++++--------------------- 1 file changed, 52 insertions(+), 48 deletions(-)