> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> 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@xxxxxx> 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@xxxxxx>
> > > Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > 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. 😊
>
> + 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.
>
either way is fine as no function is broken in bisect with above explanation.
but my slight preference is to add the label as it does make this patch
clearer for other reviewers (or with a simple explanation in the commit msg).
Thanks
Kevin
