On Thu, 26 Aug 2021 20:35:58 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> 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@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:
>
> + 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
