On Wed, Feb 19, 2025 at 05:31:42PM -0800, Nicolin Chen wrote:
> Now that iommufd does not rely on dma-iommu.c for any purpose. We can
> combine the dma-iommu.c iova_cookie and the iommufd_hwpt under the same
> union. This union is effectively 'owner data' and can be used by the
> entity that allocated the domain. Note that legacy vfio type1 flows
> continue to use dma-iommu.c for sw_msi and still need iova_cookie.
>
> Suggested-by: Jason Gunthorpe <j...@nvidia.com>
> Signed-off-by: Nicolin Chen <nicol...@nvidia.com>
> ---
> include/linux/iommu.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index e93d2e918599..99dd72998cb7 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -216,7 +216,6 @@ struct iommu_domain {
> const struct iommu_ops *owner; /* Whose domain_alloc we came from */
> unsigned long pgsize_bitmap; /* Bitmap of page sizes in use */
> struct iommu_domain_geometry geometry;
> - struct iommu_dma_cookie *iova_cookie;
> int (*iopf_handler)(struct iopf_group *group);
>
> #if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
> @@ -225,6 +224,7 @@ struct iommu_domain {
> #endif
>
> union { /* Pointer usable by owner of the domain */
> + struct iommu_dma_cookie *iova_cookie; /* dma-iommu */
> struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd */
> };
Ugh, there is a problem here:
void iommu_domain_free(struct iommu_domain *domain)
{
if (domain->type == IOMMU_DOMAIN_SVA)
mmdrop(domain->mm);
iommu_put_dma_cookie(domain);
It calls into dma-iommu for all domain types
And if !CONFIG_IRQ_MSI_IOMMU then this isn't possible to protect it
against iommufd owning the cookie union:
#if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
if (domain->sw_msi != iommu_dma_sw_msi)
return;
#endif
I came up with the below, but I will drop this patch for now you can
repost it as a cleanup series..
Jason
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 3b58244e6344a5..31d53552dc4790 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -418,6 +418,7 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
* number of PAGE_SIZE mappings necessary to cover every MSI doorbell address
* used by the devices attached to @domain.
*/
+#if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
{
struct iommu_dma_cookie *cookie;
@@ -439,6 +440,13 @@ int iommu_get_msi_cookie(struct iommu_domain *domain,
dma_addr_t base)
}
EXPORT_SYMBOL(iommu_get_msi_cookie);
+void iommu_put_msi_cookie(struct iommu_domain *domain)
+{
+ iommu_put_dma_cookie(domain);
+}
+EXPORT_SYMBOL_GPL(iommu_put_msi_cookie);
+#endif
+
/**
* iommu_put_dma_cookie - Release a domain's DMA mapping resources
* @domain: IOMMU domain previously prepared by iommu_get_dma_cookie() or
@@ -449,8 +457,10 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
struct iommu_dma_cookie *cookie = domain->iova_cookie;
struct iommu_dma_msi_page *msi, *tmp;
+#if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
if (domain->sw_msi != iommu_dma_sw_msi)
return;
+#endif
if (!cookie)
return;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 022bf96a18c5e4..f07544b290e5b1 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -456,6 +456,12 @@ static int iommu_init_device(struct device *dev, const
struct iommu_ops *ops)
return ret;
}
+static void iommu_default_domain_free(struct iommu_domain *domain)
+{
+ iommu_put_dma_cookie(domain);
+ iommu_domain_free(domain);
+}
+
static void iommu_deinit_device(struct device *dev)
{
struct iommu_group *group = dev->iommu_group;
@@ -494,7 +500,7 @@ static void iommu_deinit_device(struct device *dev)
*/
if (list_empty(&group->devices)) {
if (group->default_domain) {
- iommu_domain_free(group->default_domain);
+ iommu_default_domain_free(group->default_domain);
group->default_domain = NULL;
}
if (group->blocking_domain) {
@@ -2023,7 +2029,6 @@ void iommu_domain_free(struct iommu_domain *domain)
{
if (domain->type == IOMMU_DOMAIN_SVA)
mmdrop(domain->mm);
- iommu_put_dma_cookie(domain);
if (domain->ops->free)
domain->ops->free(domain);
}
@@ -3000,7 +3005,7 @@ static int iommu_setup_default_domain(struct iommu_group
*group,
out_free_old:
if (old_dom)
- iommu_domain_free(old_dom);
+ iommu_default_domain_free(old_dom);
return ret;
err_restore_domain:
@@ -3009,7 +3014,7 @@ static int iommu_setup_default_domain(struct iommu_group
*group,
group, old_dom, IOMMU_SET_DOMAIN_MUST_SUCCEED);
err_restore_def_domain:
if (old_dom) {
- iommu_domain_free(dom);
+ iommu_default_domain_free(dom);
group->default_domain = old_dom;
}
return ret;
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 50ebc9593c9d70..b5bb946c9c1b19 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2271,6 +2271,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
if (!iommu_attach_group(d->domain,
group->iommu_group)) {
list_add(&group->next, &d->group_list);
+ iommu_put_msi_cookie(domain->domain);
iommu_domain_free(domain->domain);
kfree(domain);
goto done;
@@ -2316,6 +2317,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
out_detach:
iommu_detach_group(domain->domain, group->iommu_group);
out_domain:
+ iommu_put_msi_cookie(domain->domain);
iommu_domain_free(domain->domain);
vfio_iommu_iova_free(&iova_copy);
vfio_iommu_resv_free(&group_resv_regions);
@@ -2496,6 +2498,7 @@ static void vfio_iommu_type1_detach_group(void
*iommu_data,
vfio_iommu_unmap_unpin_reaccount(iommu);
}
}
+ iommu_put_msi_cookie(domain->domain);
iommu_domain_free(domain->domain);
list_del(&domain->next);
kfree(domain);
@@ -2567,6 +2570,7 @@ static void vfio_release_domain(struct vfio_domain
*domain)
kfree(group);
}
+ iommu_put_msi_cookie(domain->domain);
iommu_domain_free(domain->domain);
}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 99dd72998cb7f7..082274e8ba6a3d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1534,12 +1534,16 @@ void iommu_debugfs_setup(void);
static inline void iommu_debugfs_setup(void) {}
#endif
-#ifdef CONFIG_IOMMU_DMA
+#if defined(CONFIG_IOMMU_DMA) && IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base);
+void iommu_put_msi_cookie(struct iommu_domain *domain);
#else /* CONFIG_IOMMU_DMA */
static inline int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t
base)
{
- return -ENODEV;
+ return 0;
+}
+static inline void iommu_put_msi_cookie(struct iommu_domain *domain)
+{
}
#endif /* CONFIG_IOMMU_DMA */
