On Mon, Aug 18, 2025 at 08:42:41PM -0300, Jason Gunthorpe wrote:
> On Mon, Aug 18, 2025 at 10:22:52AM -0700, Nicolin Chen wrote:
> > > Because this is a very common pattern in drivers.
> > >
> > > Once that is done we can see what calls to iommu_get_domain_for_dev()
> > > are even left,
> >
> > ... I found that in SMMUv3 driver, iommu_get_domain_for_dev() is
> > used to get the RID domain for an SVA domain:
> > arm_smmu_set_pasid()
> > arm_smmu_blocking_set_dev_pasid()
> >
> > These two are already given an "old" (SVA) domain pointer, FWIW.
> >
> > So, we may change to passing in the old domain as you suggested,
> > yet we still have to fix the iommu_get_domain_for_dev() in order
> > to reflect the RID domain correctly for the driver that calls it
> > (or even potentially) in some group->mutex locked context where
> > the RID domain might not be naturally passed in.
>
> It could probably be avoided by keeping track of more information in
> the master, but also it is not so bad to use a _locked version here.
Yes, I've thought about that. The concern is that some other place
someday may want to use iommu_get_domain_for_dev() in similar cases
but would find that it doesn't work. So it would have to duplicate
the domain pointer in its "master" structure.
Overall, having a _locked version feels cleaner to me.
> > > arguably we should be trying to eliminate this badly
> > > locked thing...
> >
> > Any suggestion?
>
> Bit by bit.. I counted 58 by grep
>
> Changing attach will get rid of alot of them
>
> Then there is stuff like this:
>
> domain = iommu_get_domain_for_dev(emu->card->dev);
> if (!domain || domain->type == IOMMU_DOMAIN_IDENTITY)
> return;
>
> Which should be more like
> if (iommu_get_translation_mode(dev) == IDENTITY)
>
> With sensible internal locking
Hmm, I feel this iommu_get_translation_mode() is somewhat the same
as the current iommu_get_domain_for_dev(). It would just return the
group->domain->type v.s. group->domain, right?
This doesn't have any UAF concern though.
> So that is another bunch. Not sure what will be left after.
I recall that some of the drivers manages their own domains, e.g.
drivers/gpu/drm/tegra/drm.c
So, they would want more out of the domain pointer than just type.
> Not saying to do all that here, just prefer we move toward that direction.
Yea.. I also think it's a bit difficult to justify the changes in
the non-iommu callers, since they are not affected by any patch in
this series.
Thanks
Nicolin
