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

[Andrew Jones]

On Tue, Jul 28, 2026 at 01:50:48PM +0100, Robin Murphy wrote:
> 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 <andrew.jones@xxxxxxxxxxxxxxxx>
> > ---
> > 
> > Sashiko reported this issue while reviewing a riscv iommu series[1].
> > I've only compile-tested this fix.
> > 
> > [1] https://sashiko.dev/#/patchset/20260724151218.965929-1-andrew.jones@xxxxxxxxxxxxxxxx
> > 
> > 
> >   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.

Agreed. I'll send a v2 with the static lock restored.

> 
> 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?

I don't think so. VFIO type1 goes through iommu_dma_sw_msi(), rather than
a vfio owned function, so there's no clean way to use a vfio lock there.
iommufd goes through its own iommufd_sw_msi().

Thanks,
drew