Re: [PATCH] iommu/dma: Restore locking around msi_page_list

On 28/07/2026 1:33 pm, Andrew Jones wrote:
> 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));

If this is still the sole place it's needed, why not just bring back the 
static lock as before? I can't see that requesting MSIs would be a 
particularly performance-sensitive or highly-contended operation to 
justify cluttering up domains with something that in one case is known 
to be unnecessary and in the other is still only for a rare corner case.

Alternatively, if IOMMUFD is able to do the right thing at the higher 
level, is there any scope for VFIO type1 to similarly serialise its own 
calls?

Thanks,
Robin.

> +
>   	msi_addr &= ~(phys_addr_t)(size - 1);
>   	list_for_each_entry(msi_page, msi_page_list, list)
>   		if (msi_page->phys == msi_addr)