| Message ID | 20210826133424.3362-5-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 Thu, 26 Aug 2021 15:34:14 +0200 Christoph Hellwig <hch@lst.de> wrote: > Factor out a helper to find or allocate the vfio_group to reduce the > spagetthi code in vfio_register_group_dev a little. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/vfio.c | 59 ++++++++++++++++++++++++++------------------- > 1 file changed, 34 insertions(+), 25 deletions(-) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index 18e4c7906d1b3f..852fe22125520d 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -823,10 +823,38 @@ 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) > +{ > + struct iommu_group *iommu_group; > + struct vfio_group *group; > + > + iommu_group = vfio_iommu_group_get(dev); > + if (!iommu_group) > + return ERR_PTR(-EINVAL); > + > + /* a found vfio_group already holds a reference to the iommu_group */ > + group = vfio_group_get_from_iommu(iommu_group); > + if (group) > + goto out_put; > + > + /* a newly created vfio_group keeps the reference. */ > + group = vfio_create_group(iommu_group); > + if (IS_ERR(group)) > + goto out_put; > + return group; > + > +out_put: > +#ifdef CONFIG_VFIO_NOIOMMU > + if (iommu_group_get_iommudata(iommu_group) == &noiommu) > + iommu_group_remove_device(dev); > +#endif When we get here via the first goto above, it doesn't match the code we're removing below. I stared at the below logic from patch 01 for a while and came to the conclusion that the only way we take that else branch is if the iommu_group already existed and was not created because how else could we find a vfio group for that iommu group otherwise? Therefore we only put the iommu group reference without removing the device, because we didn't add the device. The above assumes we created the iommu group and added the device. So I think there needs to be a separate goto target below that's reached in place of the first goto above. Thanks, Alex > + iommu_group_put(iommu_group); > + return group; > +} > + > int vfio_register_group_dev(struct vfio_device *device) > { > struct vfio_device *existing_device; > - struct iommu_group *iommu_group; > struct vfio_group *group; > > /* > @@ -836,36 +864,17 @@ int vfio_register_group_dev(struct vfio_device *device) > if (!device->dev_set) > vfio_assign_device_set(device, device); > > - iommu_group = vfio_iommu_group_get(device->dev); > - if (!iommu_group) > - return -EINVAL; > - > - group = vfio_group_get_from_iommu(iommu_group); > - if (!group) { > - group = vfio_create_group(iommu_group); > - if (IS_ERR(group)) { > -#ifdef CONFIG_VFIO_NOIOMMU > - if (iommu_group_get_iommudata(iommu_group) == &noiommu) > - iommu_group_remove_device(device->dev); > -#endif > - iommu_group_put(iommu_group); > - return PTR_ERR(group); > - } > - } else { > - /* > - * A found vfio_group already holds a reference to the > - * iommu_group. A created vfio_group keeps the reference. > - */ > - iommu_group_put(iommu_group); > - } > + group = vfio_group_find_or_alloc(device->dev); > + if (IS_ERR(group)) > + return PTR_ERR(group); > > existing_device = vfio_group_get_device(group, device->dev); > if (existing_device) { > dev_WARN(device->dev, "Device already exists on group %d\n", > - iommu_group_id(iommu_group)); > + iommu_group_id(group->iommu_group)); > vfio_device_put(existing_device); > #ifdef CONFIG_VFIO_NOIOMMU > - if (iommu_group_get_iommudata(iommu_group) == &noiommu) > + if (iommu_group_get_iommudata(group->iommu_group) == &noiommu) > iommu_group_remove_device(device->dev); > #endif > vfio_group_put(group);
On Thu, Aug 26, 2021 at 01:54:13PM -0600, Alex Williamson wrote: > On Thu, 26 Aug 2021 15:34:14 +0200 > Christoph Hellwig <hch@lst.de> wrote: > > > Factor out a helper to find or allocate the vfio_group to reduce the > > spagetthi code in vfio_register_group_dev a little. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > > drivers/vfio/vfio.c | 59 ++++++++++++++++++++++++++------------------- > > 1 file changed, 34 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > > index 18e4c7906d1b3f..852fe22125520d 100644 > > +++ b/drivers/vfio/vfio.c > > @@ -823,10 +823,38 @@ 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) > > +{ > > + struct iommu_group *iommu_group; > > + struct vfio_group *group; > > + > > + iommu_group = vfio_iommu_group_get(dev); > > + if (!iommu_group) > > + return ERR_PTR(-EINVAL); > > + > > + /* a found vfio_group already holds a reference to the iommu_group */ > > + group = vfio_group_get_from_iommu(iommu_group); > > + if (group) > > + goto out_put; > > + > > + /* a newly created vfio_group keeps the reference. */ > > + group = vfio_create_group(iommu_group); > > + if (IS_ERR(group)) > > + goto out_put; > > + return group; > > + > > +out_put: > > +#ifdef CONFIG_VFIO_NOIOMMU > > + if (iommu_group_get_iommudata(iommu_group) == &noiommu) > > + iommu_group_remove_device(dev); > > +#endif > > When we get here via the first goto above, it doesn't match the code > we're removing below. If we are in noiommu mode then the group is a new singleton group and vfio_group_get_from_iommu() cannot succeed, so the out_put cannot trigger for the noiommu path. This is all improved in patch 6 where the logic becomes clear: + iommu_group = iommu_group_get(dev); +#ifdef CONFIG_VFIO_NOIOMMU + if (!iommu_group && noiommu && !iommu_present(dev->bus)) { + /* + * 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. + */ + group = vfio_noiommu_group_alloc(dev); + if (group) { + add_taint(TAINT_USER, LOCKDEP_STILL_OK); + dev_warn(dev, "Adding kernel taint for vfio-noiommu group on device\n"); + } + return group; Eg we never do a pointless vfio_group_get_from_iommu() on a no-iommu group in the first place, we just create everything directly. It would be fine to add an extra label and then remove it in patch 6, but it is also fine this way and properly cleaned by the end. Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Friday, August 27, 2021 7:36 AM > > On Thu, Aug 26, 2021 at 01:54:13PM -0600, Alex Williamson wrote: > > On Thu, 26 Aug 2021 15:34:14 +0200 > > Christoph Hellwig <hch@lst.de> wrote: > > > > > Factor out a helper to find or allocate the vfio_group to reduce the > > > spagetthi code in vfio_register_group_dev a little. > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > > > drivers/vfio/vfio.c | 59 ++++++++++++++++++++++++++------------------- > > > 1 file changed, 34 insertions(+), 25 deletions(-) > > > > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > > > index 18e4c7906d1b3f..852fe22125520d 100644 > > > +++ b/drivers/vfio/vfio.c > > > @@ -823,10 +823,38 @@ 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) > > > +{ > > > + struct iommu_group *iommu_group; > > > + struct vfio_group *group; > > > + > > > + iommu_group = vfio_iommu_group_get(dev); > > > + if (!iommu_group) > > > + return ERR_PTR(-EINVAL); > > > + > > > + /* a found vfio_group already holds a reference to the iommu_group > */ > > > + group = vfio_group_get_from_iommu(iommu_group); > > > + if (group) > > > + goto out_put; > > > + > > > + /* a newly created vfio_group keeps the reference. */ > > > + group = vfio_create_group(iommu_group); > > > + if (IS_ERR(group)) > > > + goto out_put; > > > + return group; > > > + > > > +out_put: > > > +#ifdef CONFIG_VFIO_NOIOMMU > > > + if (iommu_group_get_iommudata(iommu_group) == &noiommu) > > > + iommu_group_remove_device(dev); > > > +#endif > > > > When we get here via the first goto above, it doesn't match the code > > we're removing below. > > If we are in noiommu mode then the group is a new singleton group and > vfio_group_get_from_iommu() cannot succeed, so the out_put cannot > trigger for the noiommu path. > > This is all improved in patch 6 where the logic becomes clear: patch 5.
On Thu, 26 Aug 2021 20:35:58 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > On Thu, Aug 26, 2021 at 01:54:13PM -0600, Alex Williamson wrote: > > On Thu, 26 Aug 2021 15:34:14 +0200 > > Christoph Hellwig <hch@lst.de> wrote: > > > > > Factor out a helper to find or allocate the vfio_group to reduce the > > > spagetthi code in vfio_register_group_dev a little. > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > > > drivers/vfio/vfio.c | 59 ++++++++++++++++++++++++++------------------- > > > 1 file changed, 34 insertions(+), 25 deletions(-) > > > > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > > > index 18e4c7906d1b3f..852fe22125520d 100644 > > > +++ b/drivers/vfio/vfio.c > > > @@ -823,10 +823,38 @@ 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) > > > +{ > > > + struct iommu_group *iommu_group; > > > + struct vfio_group *group; > > > + > > > + iommu_group = vfio_iommu_group_get(dev); > > > + if (!iommu_group) > > > + return ERR_PTR(-EINVAL); > > > + > > > + /* a found vfio_group already holds a reference to the iommu_group */ > > > + group = vfio_group_get_from_iommu(iommu_group); > > > + if (group) > > > + goto out_put; > > > + > > > + /* a newly created vfio_group keeps the reference. */ > > > + group = vfio_create_group(iommu_group); > > > + if (IS_ERR(group)) > > > + goto out_put; > > > + return group; > > > + > > > +out_put: > > > +#ifdef CONFIG_VFIO_NOIOMMU > > > + if (iommu_group_get_iommudata(iommu_group) == &noiommu) > > > + iommu_group_remove_device(dev); > > > +#endif > > > > When we get here via the first goto above, it doesn't match the code > > we're removing below. > > If we are in noiommu mode then the group is a new singleton group and > vfio_group_get_from_iommu() cannot succeed, so the out_put cannot > trigger for the noiommu path. > > This is all improved in patch 6 where the logic becomes clear: > > + iommu_group = iommu_group_get(dev); > +#ifdef CONFIG_VFIO_NOIOMMU > + if (!iommu_group && noiommu && !iommu_present(dev->bus)) { > + /* > + * 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. > + */ > + group = vfio_noiommu_group_alloc(dev); > + if (group) { > + add_taint(TAINT_USER, LOCKDEP_STILL_OK); > + dev_warn(dev, "Adding kernel taint for vfio-noiommu group on device\n"); > + } > + return group; > > Eg we never do a pointless vfio_group_get_from_iommu() on a no-iommu > group in the first place, we just create everything directly. > > It would be fine to add an extra label and then remove it in patch 6, > but it is also fine this way and properly cleaned by the end. I agree that it's resolved later in the series, but it's confusing here, particularly because in patch 1 we need to come to the conclusion that path is unreachable, thus the different return paths, then we ignore it here with a common exit. Thanks, Alex
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index 18e4c7906d1b3f..852fe22125520d 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -823,10 +823,38 @@ 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) +{ + struct iommu_group *iommu_group; + struct vfio_group *group; + + iommu_group = vfio_iommu_group_get(dev); + if (!iommu_group) + return ERR_PTR(-EINVAL); + + /* a found vfio_group already holds a reference to the iommu_group */ + group = vfio_group_get_from_iommu(iommu_group); + if (group) + goto out_put; + + /* a newly created vfio_group keeps the reference. */ + group = vfio_create_group(iommu_group); + if (IS_ERR(group)) + goto out_put; + return group; + +out_put: +#ifdef CONFIG_VFIO_NOIOMMU + if (iommu_group_get_iommudata(iommu_group) == &noiommu) + iommu_group_remove_device(dev); +#endif + iommu_group_put(iommu_group); + return group; +} + int vfio_register_group_dev(struct vfio_device *device) { struct vfio_device *existing_device; - struct iommu_group *iommu_group; struct vfio_group *group; /* @@ -836,36 +864,17 @@ int vfio_register_group_dev(struct vfio_device *device) if (!device->dev_set) vfio_assign_device_set(device, device); - iommu_group = vfio_iommu_group_get(device->dev); - if (!iommu_group) - return -EINVAL; - - group = vfio_group_get_from_iommu(iommu_group); - if (!group) { - group = vfio_create_group(iommu_group); - if (IS_ERR(group)) { -#ifdef CONFIG_VFIO_NOIOMMU - if (iommu_group_get_iommudata(iommu_group) == &noiommu) - iommu_group_remove_device(device->dev); -#endif - iommu_group_put(iommu_group); - return PTR_ERR(group); - } - } else { - /* - * A found vfio_group already holds a reference to the - * iommu_group. A created vfio_group keeps the reference. - */ - iommu_group_put(iommu_group); - } + group = vfio_group_find_or_alloc(device->dev); + if (IS_ERR(group)) + return PTR_ERR(group); existing_device = vfio_group_get_device(group, device->dev); if (existing_device) { dev_WARN(device->dev, "Device already exists on group %d\n", - iommu_group_id(iommu_group)); + iommu_group_id(group->iommu_group)); vfio_device_put(existing_device); #ifdef CONFIG_VFIO_NOIOMMU - if (iommu_group_get_iommudata(iommu_group) == &noiommu) + if (iommu_group_get_iommudata(group->iommu_group) == &noiommu) iommu_group_remove_device(device->dev); #endif vfio_group_put(group);