Re: [PATCH v3 3/5] iommu: Add iommu_get_domain_for_dev_locked() helper

[Nicolin Chen <nicolinc@nvidia.com>]

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