linux-kernel - Re: [PATCH v3 3/5] iommu: Add iommu_get_domain_for_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aKSyzI9Xz3J0nhfk@Asurada-Nvidia>
Date: Tue, 19 Aug 2025 10:22:20 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: <robin.murphy@....com>, <joro@...tes.org>, <bhelgaas@...gle.com>,
	<will@...nel.org>, <robin.clark@....qualcomm.com>, <yong.wu@...iatek.com>,
	<matthias.bgg@...il.com>, <angelogioacchino.delregno@...labora.com>,
	<thierry.reding@...il.com>, <vdumpa@...dia.com>, <jonathanh@...dia.com>,
	<rafael@...nel.org>, <lenb@...nel.org>, <kevin.tian@...el.com>,
	<yi.l.liu@...el.com>, <baolu.lu@...ux.intel.com>,
	<linux-arm-kernel@...ts.infradead.org>, <iommu@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
	<linux-mediatek@...ts.infradead.org>, <linux-tegra@...r.kernel.org>,
	<linux-acpi@...r.kernel.org>, <linux-pci@...r.kernel.org>,
	<patches@...ts.linux.dev>, <pjaroszynski@...dia.com>, <vsethi@...dia.com>,
	<helgaas@...nel.org>, <etzhao1900@...il.com>
Subject: Re: [PATCH v3 3/5] iommu: Add iommu_get_domain_for_dev_locked()
 helper

On Tue, Aug 19, 2025 at 09:52:49AM -0300, Jason Gunthorpe wrote:
> On Mon, Aug 18, 2025 at 10:09:11PM -0700, Nicolin Chen wrote:
> > 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.
> 
> We probably need the locked version, but it just shouldn't be called very
> much..

OK. Let's have one patch upgrading the attach_dev to pass in the
old domain pointer (aligning with the SVA version of attach_dev),
and another patch adding the _locked version that then will have
very limited callers.

> > > 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.
> 
> Yes, no UAF concern is the point

I see.

> > > 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.
> 
> This looks like it wants an 'is currently attached' operation

That's certainly one of the wide use cases. And I think we could
have an IOMMU_DOMAIN_NONE to fit that into the type helper.

Yet, I also see some other cases that cannot be helped with the
type function. Just listing a few:

1) domain matching (and type)
drivers/gpu/drm/tegra/drm.c:965:        if (domain && domain->type != IOMMU_DOMAIN_IDENTITY &&
drivers/gpu/drm/tegra/drm.c:966:            domain != tegra->domain)
drivers/gpu/drm/tegra/drm.c-967-                return 0;

2) page size
drivers/gpu/drm/arm/malidp_planes.c:307:	mmu_dom = iommu_get_domain_for_dev(mp->base.dev->dev);
drivers/gpu/drm/arm/malidp_planes.c-308-	if (mmu_dom)
drivers/gpu/drm/arm/malidp_planes.c-309-		return mmu_dom->pgsize_bitmap;

3) iommu_iova_to_phys
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c:2597:	dom = iommu_get_domain_for_dev(adev->dev);
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2598-
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2599-	while (size) {
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2600-		phys_addr_t addr = *pos & PAGE_MASK;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2601-		loff_t off = *pos & ~PAGE_MASK;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2602-		size_t bytes = PAGE_SIZE - off;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2603-		unsigned long pfn;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2604-		struct page *p;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2605-		void *ptr;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2606-
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2607-		bytes = min(bytes, size);
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2608-
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c:2609:		addr = dom ? iommu_iova_to_phys(dom, addr) : addr;

4) map/unmap
drivers/net/ipa/ipa_mem.c:465:  domain = iommu_get_domain_for_dev(dev);
drivers/net/ipa/ipa_mem.c-466-  if (!domain) {
drivers/net/ipa/ipa_mem.c-467-          dev_err(dev, "no IOMMU domain found for IMEM\n");
drivers/net/ipa/ipa_mem.c-468-          return -EINVAL;
drivers/net/ipa/ipa_mem.c-469-  }
drivers/net/ipa/ipa_mem.c-470-
drivers/net/ipa/ipa_mem.c-471-  /* Align the address down and the size up to page boundaries */
drivers/net/ipa/ipa_mem.c-472-  phys = addr & PAGE_MASK;
drivers/net/ipa/ipa_mem.c-473-  size = PAGE_ALIGN(size + addr - phys);
drivers/net/ipa/ipa_mem.c-474-  iova = phys;    /* We just want a direct mapping */
drivers/net/ipa/ipa_mem.c-475-
drivers/net/ipa/ipa_mem.c-476-  ret = iommu_map(domain, iova, phys, size, IOMMU_READ | IOMMU_WRITE,
...
drivers/net/ipa/ipa_mem.c:495:  domain = iommu_get_domain_for_dev(dev);
drivers/net/ipa/ipa_mem.c-496-  if (domain) {
drivers/net/ipa/ipa_mem.c-497-          size_t size;
drivers/net/ipa/ipa_mem.c-498-
drivers/net/ipa/ipa_mem.c-499-          size = iommu_unmap(domain, ipa->imem_iova, ipa->imem_size);


We could probably invent something for case 1-3. But for case 4,
it feels that iommu_get_domain_for_dev() is the only helper..

Thanks
Nicolin