Re: [PATCH 04/14] vfio: factor out a vfio_group_find_or_alloc helper

[Jason Gunthorpe <jgg@nvidia.com>]

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