linux-kernel - Re: [PATCH v3 1/3] iommu: Sort out domain user data

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: <Z8oBJJz27/Z8P0vR@Asurada-Nvidia>
Date: Thu, 6 Mar 2025 12:10:12 -0800
From: Nicolin Chen <nicolinc@...dia.com>
To: "Tian, Kevin" <kevin.tian@...el.com>
CC: "jgg@...dia.com" <jgg@...dia.com>, "robin.murphy@....com"
	<robin.murphy@....com>, "joro@...tes.org" <joro@...tes.org>,
	"will@...nel.org" <will@...nel.org>, "iommu@...ts.linux.dev"
	<iommu@...ts.linux.dev>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/3] iommu: Sort out domain user data

On Thu, Mar 06, 2025 at 05:59:03AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@...dia.com>
> > Sent: Tuesday, March 4, 2025 4:53 AM
> > 
> > From: Robin Murphy <robin.murphy@....com>
> > 
> > When DMA/MSI cookies were made first-class citizens back in commit
> > 46983fcd67ac ("iommu: Pull IOVA cookie management into the core"), there
> > was no real need to further expose the two different cookie types.
> > However, now that IOMMUFD wants to add a third type of MSI-mapping
> > cookie, we do have a nicely compelling reason to properly dismabiguate
> > things at the domain level beyond just vaguely guessing from the domain
> > type.
> > 
> > Meanwhile, we also effectively have another "cookie" in the form of the
> > anonymous union for other user data, which isn't much better in terms of
> > being vague and unenforced. The fact is that all these cookie types are
> > mutually exclusive, in the sense that combining them makes zero sense
> > and/or would be catastrophic (iommu_set_fault_handler() on an SVA
> > domain, anyone?) - the only combination which *might* be reasonable is
> > perhaps a fault handler and an MSI cookie, but nobody's doing that at
> > the moment, so let's rule it out as well for the sake of being clear and
> > robust. To that end, we pull DMA and MSI cookies apart a little more,
> > mostly to clear up the ambiguity at domain teardown, then for clarity
> > (and to save a little space), move them into the union, whose ownership
> > we can then properly describe and enforce entirely unambiguously.
> > 
> > Signed-off-by: Robin Murphy <robin.murphy@....com>
> > [nicolinc: rebase on latest tree; use prefix IOMMU_COOKIE_; merge unions
> >            in iommu_domain; add IOMMU_COOKIE_IOMMUFD for
> > iommufd_hwpt]
> > Signed-off-by: Nicolin Chen <nicolinc@...dia.com>
> 
> Reviewed-by: Kevin Tian <kevin.tian@...el.com>
> 
> with a nit:
> 
> >  	msi_addr &= ~(phys_addr_t)(size - 1);
> > -	list_for_each_entry(msi_page, &cookie->msi_page_list, list)
> > +	list_for_each_entry(msi_page, cookie_msi_pages(domain), list)
> >  		if (msi_page->phys == msi_addr)
> >  			return msi_page;
> > 
> 
> Above checks cookie type in every iteration. Better save the list
> pointer to a local variable.

Ack.

--------------------------------------------------------------
@@ -1793,2 +1793,3 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 {
+       struct list_head *msi_page_list = cookie_msi_pages(domain);
        struct iommu_dma_msi_page *msi_page;
@@ -1799,3 +1800,3 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
        msi_addr &= ~(phys_addr_t)(size - 1);
-       list_for_each_entry(msi_page, cookie_msi_pages(domain), list)
+       list_for_each_entry(msi_page, msi_page_list, list)
                if (msi_page->phys == msi_addr)
@@ -1817,3 +1818,3 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
        msi_page->iova = iova;
-       list_add(&msi_page->list, cookie_msi_pages(domain));
+       list_add(&msi_page->list, msi_page_list);
        return msi_page;
--------------------------------------------------------------

Thanks
Nicolin