Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces

[Baolu Lu]

On 2022/5/12 19:51, Jason Gunthorpe wrote:
> On Thu, May 12, 2022 at 11:02:39AM +0800, Baolu Lu wrote:
> > > > +       mutex_lock(&group->mutex);
> > > > +       domain = xa_load(&group->pasid_array, pasid);
> > > > +       if (domain && domain->type != type)
> > > > +               domain = NULL;
> > > > +       mutex_unlock(&group->mutex);
> > > > +       iommu_group_put(group);
> > > > +
> > > > +       return domain;
> > > This is bad locking, group->pasid_array values cannot be taken outside
> > > the lock.
> >
> > It's not iommu core, but SVA (or other feature components) that manage
> > the life cycle of a domain. The iommu core only provides a place to
> > store the domain pointer. The feature components are free to fetch their
> > domain pointers from iommu core as long as they are sure that the domain
> > is alive during use.
>
> I'm not convinced.

I'm sorry, I may not have explained it clearly. :-)

This helper is safe for uses inside the IOMMU subsystem. We could trust
ourselves that nobody will abuse this helper to obtain domains belonging
to others as the pasid has been allocated for SVA code. No other code
should be able to setup another type of domain for this pasid. The SVA
code has its own lock mechanism to avoid using after free.

Please correct me if I missed anything. :-) By the way, I can see some
similar helpers in current IOMMU core. For example,

struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
{
        struct iommu_domain *domain;
        struct iommu_group *group;

        group = iommu_group_get(dev);
        if (!group)
                return NULL;

        domain = group->domain;

        iommu_group_put(group);

        return domain;
}
EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev);

Best regards,
baolu