[PATCH] iommu/dma: Restore locking around msi_page_list

Unlike a group's default domain, which is always freshly allocated
and privately owned (iommu_group_alloc_default_domain()), VFIO type1's
legacy container merges any newly attached group into an existing
domain whenever their iommu_ops and cache-coherency enforcement match.

iommu_dma_get_msi_page() only asserts the caller's own group mutex is
held (iommu_group_mutex_assert()). On an IOMMU that publishes
IOMMU_RESV_SW_MSI, e.g. ARM SMMU, a VM with two such devices assigned
through the legacy container can have their guest drivers probe and
allocate MSIs in parallel; each host-side VFIO_DEVICE_SET_IRQS lands
on a different device fd and group mutex, but both devices' domains
are the same merged domain, so both can enter
iommu_dma_get_msi_page() concurrently and corrupt msi_page_list.

commit 288683c92b1a ("iommu: Make iommu_dma_prepare_msi() into a
generic operation") dropped the prior msi_prepare_lock on the
reasoning that "each iommu_domain is unique to a group," which holds
for default domains but not this VFIO type1 case. iommufd already
handles its analogous list correctly: iommufd_sw_msi_get_map()'s
sw_msi_list is explicitly ctx-wide and protected by ctx->sw_msi_lock,
not a per-group lock. Give dma-iommu's cookie the same treatment: a
mutex scoped to the domain, held around the lookup-or-insert in
iommu_dma_get_msi_page().

Fixes: 288683c92b1a ("iommu: Make iommu_dma_prepare_msi() into a generic operation")
Signed-off-by: Andrew Jones <[email protected]>
---

Sashiko reported this issue while reviewing a riscv iommu series[1].
I've only compile-tested this fix.

[1] https://sashiko.dev/#/patchset/[email protected]


 drivers/iommu/dma-iommu.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 9abaec0703ef..0b5da4579aaf 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -58,6 +58,7 @@ struct iommu_dma_options {
 struct iommu_dma_cookie {
 	struct iova_domain iovad;
 	struct list_head msi_page_list;
+	struct mutex msi_lock;
 	/* Flush queue */
 	union {
 		struct iova_fq *single_fq;
@@ -80,6 +81,7 @@ struct iommu_dma_cookie {
 struct iommu_dma_msi_cookie {
 	dma_addr_t msi_iova;
 	struct list_head msi_page_list;
+	struct mutex msi_lock;
 };
 
 static DEFINE_STATIC_KEY_FALSE(iommu_deferred_attach_enabled);
@@ -378,6 +380,7 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
 		return -ENOMEM;
 
 	INIT_LIST_HEAD(&cookie->msi_page_list);
+	mutex_init(&cookie->msi_lock);
 	domain->cookie_type = IOMMU_COOKIE_DMA_IOVA;
 	domain->iova_cookie = cookie;
 	return 0;
@@ -411,6 +414,7 @@ int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
 
 	cookie->msi_iova = base;
 	INIT_LIST_HEAD(&cookie->msi_page_list);
+	mutex_init(&cookie->msi_lock);
 	domain->cookie_type = IOMMU_COOKIE_DMA_MSI;
 	domain->msi_cookie = cookie;
 	return 0;
@@ -2196,6 +2200,18 @@ static struct list_head *cookie_msi_pages(const struct iommu_domain *domain)
 	}
 }
 
+static struct mutex *cookie_msi_lock(const struct iommu_domain *domain)
+{
+	switch (domain->cookie_type) {
+	case IOMMU_COOKIE_DMA_IOVA:
+		return &domain->iova_cookie->msi_lock;
+	case IOMMU_COOKIE_DMA_MSI:
+		return &domain->msi_cookie->msi_lock;
+	default:
+		BUG();
+	}
+}
+
 static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 		phys_addr_t msi_addr, struct iommu_domain *domain)
 {
@@ -2205,6 +2221,16 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
 	size_t size = cookie_msi_granule(domain);
 
+	/*
+	 * A VFIO type1 container can attach the same domain to devices in
+	 * different iommu groups, each serialised by its own group mutex, so
+	 * the group mutex held by iommu_dma_sw_msi()'s caller isn't enough to
+	 * serialise msi_page_list lookups and insertions against each other
+	 * here; take the cookie's own lock instead, scoped to just this
+	 * domain.
+	 */
+	guard(mutex)(cookie_msi_lock(domain));
+
 	msi_addr &= ~(phys_addr_t)(size - 1);
 	list_for_each_entry(msi_page, msi_page_list, list)
 		if (msi_page->phys == msi_addr)
-- 
2.43.0