Re: [PATCH v2 7/7] iommu: Turn iova_cookie to dma-iommu private pointer

[Robin Murphy <robin.murphy@arm.com>]

On 2025-02-21 2:39 pm, Jason Gunthorpe wrote:
> 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 <jgg@nvidia.com>
>> Signed-off-by: Nicolin Chen <nicolinc@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..

Eww... What's the issue with just checking the domain type in 
iommu_put_dma_cookie()? Is is that IOMMUFD and VFIO type 1 are both 
doing their own different thing with IOMMU_DOMAIN_UNMANAGED?

In general it seems like a bad smell to have a union in a structure with 
not enough information within that structire itself to know which union 
member is valid... :/

Thanks,
Robin.

> 
> 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 */
>