iommu/dma: Restore locking around msi_page_list
Subsystems (2)
PATCH v2 1/1 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. Restore the static lock, since it's only guarding a corner case and will likely never be contended.
iommufd avoids the equivalent problem by having its own callers (iommufd_sw_map_msi()) take a ctx-wide sw_msi_lock before ever reaching the shared list. VFIO type1 can't mirror that since it dispatches to iommu_dma_sw_msi() which is outside VFIO's jurisdiction.
---
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]
v2:
- switched back to statick lock as 288683c92b1a had [Robin]
drivers/iommu/dma-iommu.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
drivers/iommu/dma-iommu.c +13 -0
@@ -2204,6 +2204,19 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,dma_addr_t iova;int prot =IOMMU_WRITE |IOMMU_NOEXEC |IOMMU_MMIO ;size_t size =cookie_msi_granule (domain); +static DEFINE_MUTEX (msi_prepare_lock); + +/* +* Normally a device's default domain is only ever attached to that +* device's own group, and the group mutex held by +* iommu_group_mutex_assert()'s callers is enough on its own. A VFIO +* type1 container is the one case that breaks that assumption: it +* can merge devices from different groups onto one domain, so two +* devices' group mutexes don't serialize each other here. A static +* lock is sufficient due to the expectation that this is a corner +* case that will never be contended in practice. +*/ +guard (mutex)(&msi_prepare_lock); msi_addr &= ~(phys_addr_t)(size -1 );list_for_each_entry (msi_page, msi_page_list, list)
Signature · 1 line
2.43.0
Quoted 21 lines · Andrew Jones
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 theiriommu_opsand 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 publishesIOMMU_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-sideVFIO_DEVICE_SET_IRQSlands on a different device fd and group mutex, but both devices' domains are the same merged domain, so both can enteriommu_dma_get_msi_page()concurrently and corruptmsi_page_list.commit
288683c92b1a("iommu: Makeiommu_dma_prepare_msi()into a generic operation") dropped the priormsi_prepare_lockon the reasoning that "eachiommu_domainis unique to a group," which holds for default domains but not this VFIO type1 case. Restore the static lock, since it's only guarding a corner case and will likely never be contended.
Quoted 4 lines
iommufd avoids the equivalent problem by having its own callers (
iommufd_sw_map_msi()) take a ctx-widesw_msi_lockbefore ever reaching the shared list. VFIO type1 can't mirror that since it dispatches toiommu_dma_sw_msi()which is outside VFIO's jurisdiction.
It occurs to me that it's also not impossible to explicitly attach a group to another group's DMA domain either, so in theory I think the concern could technically apply in both cases anyway. Plus it's not like anyone ever claimed any issue when the locking was in this path before, so I reckon it's the right thing to do for peace of mind.
Thanks,
Robin.
Quoted 39 lines
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]
v2:
- switched back to statick lock as288683c92b1ahad [Robin]drivers/iommu/dma-iommu.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)drivers/iommu/dma-iommu.c +13
-0@@ -2204,6 +2204,19 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,dma_addr_t iova;int prot =IOMMU_WRITE |IOMMU_NOEXEC |IOMMU_MMIO ;size_t size =cookie_msi_granule (domain); +static DEFINE_MUTEX (msi_prepare_lock); + +/* +* Normally a device's default domain is only ever attached to that +* device's own group, and the group mutex held by +* iommu_group_mutex_assert()'s callers is enough on its own. A VFIO +* type1 container is the one case that breaks that assumption: it +* can merge devices from different groups onto one domain, so two +* devices' group mutexes don't serialize each other here. A static +* lock is sufficient due to the expectation that this is a corner +* case that will never be contended in practice. +*/ +guard (mutex)(&msi_prepare_lock); msi_addr &= ~(phys_addr_t)(size -1 );list_for_each_entry (msi_page, msi_page_list, list)
Quoted 21 lines · Andrew Jones
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 theiriommu_opsand 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 publishesIOMMU_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-sideVFIO_DEVICE_SET_IRQSlands on a different device fd and group mutex, but both devices' domains are the same merged domain, so both can enteriommu_dma_get_msi_page()concurrently and corruptmsi_page_list.commit
288683c92b1a("iommu: Makeiommu_dma_prepare_msi()into a generic operation") dropped the priormsi_prepare_lockon the reasoning that "eachiommu_domainis unique to a group," which holds for default domains but not this VFIO type1 case. Restore the static lock, since it's only guarding a corner case and will likely never be contended.
Quoted 7 lines
iommufd avoids the equivalent problem by having its own callers (
iommufd_sw_map_msi()) take a ctx-widesw_msi_lockbefore ever reaching the shared list. VFIO type1 can't mirror that since it dispatches toiommu_dma_sw_msi()which is outside VFIO's jurisdiction.Fixes 288683c92b1a ("iommu: Make iommu_dma_prepare_msi() into a generic operation")Signed-off-by Andrew Jones <[email protected]>
Thanks,
Nutty
Quoted 37 lines
---
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]
v2:
- switched back to statick lock as288683c92b1ahad [Robin]drivers/iommu/dma-iommu.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)drivers/iommu/dma-iommu.c +13
-0@@ -2204,6 +2204,19 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,dma_addr_t iova;int prot =IOMMU_WRITE |IOMMU_NOEXEC |IOMMU_MMIO ;size_t size =cookie_msi_granule (domain); +static DEFINE_MUTEX (msi_prepare_lock); + +/* +* Normally a device's default domain is only ever attached to that +* device's own group, and the group mutex held by +* iommu_group_mutex_assert()'s callers is enough on its own. A VFIO +* type1 container is the one case that breaks that assumption: it +* can merge devices from different groups onto one domain, so two +* devices' group mutexes don't serialize each other here. A static +* lock is sufficient due to the expectation that this is a corner +* case that will never be contended in practice. +*/ +guard (mutex)(&msi_prepare_lock); msi_addr &= ~(phys_addr_t)(size -1 );list_for_each_entry (msi_page, msi_page_list, list)
Quoted 7 lines
drivers/iommu/dma-iommu.c +3
-0@@ -2204,6 +2204,19 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,dma_addr_t iova;int prot =IOMMU_WRITE |IOMMU_NOEXEC |IOMMU_MMIO ;size_t size =cookie_msi_granule (domain); +static DEFINE_MUTEX (msi_prepare_lock); + +/*
drivers/iommu/dma-iommu.c static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev, · line 2219
Quoted 10 lines
drivers/iommu/dma-iommu.c +10
-0+* Normally a device's default domain is only ever attached to that +* device's own group, and the group mutex held by +* iommu_group_mutex_assert()'s callers is enough on its own. A VFIO +* type1 container is the one case that breaks that assumption: it +* can merge devices from different groups onto one domain, so two +* devices' group mutexes don't serialize each other here. A static +* lock is sufficient due to the expectation that this is a corner +* case that will never be contended in practice. +*/ +guard (mutex)(&msi_prepare_lock);
It looks OK to me
But I wondered what the original comment ment:
- /*
- * In fact the whole prepare operation should already be serialised by - * irq_domain_mutex further up the callchain, but that's pretty subtle - * on its own, so consider this locking as failsafe documentation... - */
I sicced AI on the question and it pointed out that when this was originally done in commit c18647900ec8 ("iommu/dma: Relax locking in iommu_dma_prepare_msi()") there was in fact a global irq_domain_mutex being held in this callchain.
But that was changed by commit 9dbb8e3452ab ("irqdomain: Switch to per-domain locking") and it became a per-domain lock
And then this case probably has multiple irq domains for all the different VFIO devices.
So the comment became out of date and I didn't double check my self it was still accurate when I removed it in 288683c92b1a ..
Signature · 1 line
Jason
Quoted 2 lines · Andrew Jones
drivers/iommu/dma-iommu.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
Applied, thanks.