[v2,1/6] ARM/dma-mapping: Remove iommu_detach_device()

  iommu_detach_device() attaches the default domain to the device, or if
default domain is not supported by the IOMMU driver, it calls its
set_platform_dma_ops callback. If the default domain is supported or
the IOMMU driver is not iommu-dma aware, iommu_detach_device() is
actually a noop.

The 64-bit ARM drivers always support default domain and iommu-dma is
even not enabled for 32-bit ARM. This turns out that iommu_detach_device()
is always a noop in arm_iommu_detach_device(). Remove it to avoid dead
code.

The bonus is that it also removes a obstacle of arm_iommu_detach_device()
re-entering the iommu core during release_device. With this removed, the
iommu core code could be simplified a lot.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 arch/arm/mm/dma-mapping.c | 1 -
 1 file changed, 1 deletion(-)
  

On Fri, Feb 17, 2023 at 05:47:31PM +0800, Lu Baolu wrote:
> iommu_detach_device() attaches the default domain to the device, or if
> default domain is not supported by the IOMMU driver, it calls its
> set_platform_dma_ops callback. If the default domain is supported or
> the IOMMU driver is not iommu-dma aware, iommu_detach_device() is
> actually a noop.
> 
> The 64-bit ARM drivers always support default domain and iommu-dma is
> even not enabled for 32-bit ARM. This turns out that iommu_detach_device()
> is always a noop in arm_iommu_detach_device(). Remove it to avoid dead
> code.

This isn't entirely right..

The purpose of the iommu_detach_device here should be to make the
iommu driver stop using the domain that it is about to free. It cannot
be a NOP or it is a UAF.

I think things have become confused.

What we decided is that NULL domain is supposed to mean the DMA
translation is controlled by the platform, the concept is modeled
after S390's private iommu implementation. This means the current
IOMMU translation is invisibly set to something that matches the
device's dma_ops.

arm_iommu doesn't work that way, it allocates and assigns domains so
when the platform DMA ops are in control the group->domain is not NULL
- which is the opposite of S390's design. Further when arm_iommu asks
for a NULL domain it doesn't mean "put it back to platform DMA ops" it
really means "park the IOMMU it is not being used anymore"

So.. Blah - we had two meanings for group->domain = NULL and didn't
quite get it right.

IMHO the way to make sense of this is to always have a domain attached
and remove group->domain = NULL and set_platform_dma entirely. If the
driver doesn't want to use dma_iommu then it should provide its own
iommu_domain that it wants attached whenever the iommu API is not
being used.

You can see this in the exynos fix because what it is doing is calling
__sysmmu_disable() from set_platform_dma which is either IDENTITY or
BLOCKING IOMMU behavior.

Then we can document what the idle domain is supposed to be doing in
each of the drivers, but the core code is logical and doesn't have a
confusing overloaded domain = NULL case.

Something like this as a starting idea of how the drivers could look.

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index b0cde22119875e..143d1abcae2641 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -948,6 +948,20 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
 		&pagetable);
 }
 
+static struct iommu_domain_ops exynos_private_ops = {
+	.attach_dev_nofail = &exynos_iommu_detach_device,
+};
+
+static struct iommu_domain exynos_private_domain = {
+	/*
+	 * This is private because nobody knew what __sysmmu_disable() does.
+	 * When someone figures that out this should be made blocking or
+	 * identity
+	 */
+	.type = IOMMU_DOMAN_PRIVATE,
+	.ops = &exynos_private_ops,
+};
+
 static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 				   struct device *dev)
 {
@@ -1400,11 +1414,11 @@ static const struct iommu_ops exynos_iommu_ops = {
 	.device_group = generic_device_group,
 	.probe_device = exynos_iommu_probe_device,
 	.release_device = exynos_iommu_release_device,
+	.idle_domain = IS_ENABLED(CONFIG_ARM) ? &exynos_private_domain : NULL,
 	.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
 	.of_xlate = exynos_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= exynos_iommu_attach_device,
-		.detach_dev	= exynos_iommu_detach_device,
 		.map		= exynos_iommu_map,
 		.unmap		= exynos_iommu_unmap,
 		.iova_to_phys	= exynos_iommu_iova_to_phys,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index de91dd88705bd3..b71a1667f43bd5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -387,14 +387,13 @@ int iommu_probe_device(struct device *dev)
 		goto err_release;
 	}
 
-	/*
-	 * Try to allocate a default domain - needs support from the
-	 * IOMMU driver. There are still some drivers which don't
-	 * support default domains, so the return value is not yet
-	 * checked.
-	 */
 	mutex_lock(&group->mutex);
-	iommu_alloc_default_domain(group, dev);
+	ret = iommu_alloc_default_domain(group, dev);
+	if (ret) {
+		mutex_unlock(&group->mutex);
+		iommu_group_put(group);
+		goto err_release;
+	}
 
 	/*
 	 * If device joined an existing group which has been claimed, don't
@@ -1645,7 +1644,15 @@ static int iommu_alloc_default_domain(struct iommu_group *group,
 
 	type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type;
 
-	return iommu_group_alloc_default_domain(dev->bus, group, type);
+	if (!iommu_group_alloc_default_domain(dev->bus, group, type))
+		return 0;
+
+	/*
+	 * Driver must support default domains or provide an legacy idle domain
+	 */
+	if (WARN_ON(!dev_iommu_ops(dev)->idle_domain))
+		return -EINVAL;
+	return 0;
 }
 
 /**
@@ -2172,17 +2179,22 @@ static int __iommu_group_set_domain(struct iommu_group *group,
 		return 0;
 
 	/*
-	 * New drivers should support default domains and so the detach_dev() op
-	 * will never be called. Otherwise the NULL domain represents some
-	 * platform specific behavior.
+	 * New drivers should support default domains, otherwise the driver
+	 * must provide a domain to be attached when the iommu subsystem
+	 * is not using the device. The purpose of this domain depends
+	 * on how the iommu driver is operating the dma_ops.
 	 */
 	if (!new_domain) {
-		if (WARN_ON(!group->domain->ops->detach_dev))
+		struct group_device *grp_dev;
+		const struct iommu_ops *ops;
+
+		grp_dev = list_first_entry(&group->devices, struct group_device,
+					   list);
+		ops = dev_iommu_ops(grp_dev->dev);
+
+		if (WARN_ON(!ops->idle_domain))
 			return -EINVAL;
-		__iommu_group_for_each_dev(group, group->domain,
-					   iommu_group_do_detach_device);
-		group->domain = NULL;
-		return 0;
+		new_domain = ops->idle_domain;
 	}
 
 	/*
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 46e1347bfa2286..a17069368d70d9 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -91,6 +91,7 @@ struct iommu_domain_geometry {
 				 __IOMMU_DOMAIN_DMA_API |	\
 				 __IOMMU_DOMAIN_DMA_FQ)
 #define IOMMU_DOMAIN_SVA	(__IOMMU_DOMAIN_SVA)
+#define IOMMU_DOMAN_PRIVATE	(1 << 5)
 
 struct iommu_domain {
 	unsigned type;
@@ -272,7 +273,18 @@ struct iommu_ops {
 			     struct iommu_fault_event *evt,
 			     struct iommu_page_response *msg);
 
+	/*
+	 * driver wants to use dma_ops provided by dma-iommu.c, return
+	 * the recommended initial IDENTITY/DMA domain.
+	 */
 	int (*def_domain_type)(struct device *dev);
+
+	/*
+	 * Driver is not using dma-iommu.c, when the iommu core is not
+	 * using the device it will attach it to this domain.
+	 */
+	struct iommu_domain *idle_domain;
+
 	void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid);
 
 	const struct iommu_domain_ops *default_domain_ops;
@@ -315,6 +327,7 @@ struct iommu_ops {
  * @free: Release the domain after use.
  */
 struct iommu_domain_ops {
+	void (*attach_dev_nofail)(struct iommu_domain *domain, struct device *dev);
 	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
 	void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
 	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
  

On 2/17/23 11:39 PM, Jason Gunthorpe wrote:
> On Fri, Feb 17, 2023 at 05:47:31PM +0800, Lu Baolu wrote:
>> iommu_detach_device() attaches the default domain to the device, or if
>> default domain is not supported by the IOMMU driver, it calls its
>> set_platform_dma_ops callback. If the default domain is supported or
>> the IOMMU driver is not iommu-dma aware, iommu_detach_device() is
>> actually a noop.
>>
>> The 64-bit ARM drivers always support default domain and iommu-dma is
>> even not enabled for 32-bit ARM. This turns out that iommu_detach_device()
>> is always a noop in arm_iommu_detach_device(). Remove it to avoid dead
>> code.
> 
> This isn't entirely right..
> 
> The purpose of the iommu_detach_device here should be to make the
> iommu driver stop using the domain that it is about to free. It cannot
> be a NOP or it is a UAF.
> 
> I think things have become confused.
> 
> What we decided is that NULL domain is supposed to mean the DMA
> translation is controlled by the platform, the concept is modeled
> after S390's private iommu implementation. This means the current
> IOMMU translation is invisibly set to something that matches the
> device's dma_ops.

Yes. This matches what I understood.

> 
> arm_iommu doesn't work that way, it allocates and assigns domains so
> when the platform DMA ops are in control the group->domain is not NULL

This is what the iommu core assumes, right? Any iommu group should
always has a domain attached, default domain, blocking domain or driver-
owned unmanaged domain. The iommu core just switches between different
domains.

> - which is the opposite of S390's design. Further when arm_iommu asks
> for a NULL domain it doesn't mean "put it back to platform DMA ops" it
> really means "park the IOMMU it is not being used anymore"

This is what identity domain and blocking domains were designed to do,
right?

If my understanding is right, ARM presumably could implement the
identity default domain and blocking domain. With that implemented,
iommu_attach/detac_device() could be removed from drivers and everything
then could go through the iommu core.

Best regards,
baolu

> 
> So.. Blah - we had two meanings for group->domain = NULL and didn't
> quite get it right.
> 
> IMHO the way to make sense of this is to always have a domain attached
> and remove group->domain = NULL and set_platform_dma entirely. If the
> driver doesn't want to use dma_iommu then it should provide its own
> iommu_domain that it wants attached whenever the iommu API is not
> being used.
> 
> You can see this in the exynos fix because what it is doing is calling
> __sysmmu_disable() from set_platform_dma which is either IDENTITY or
> BLOCKING IOMMU behavior.
> 
> Then we can document what the idle domain is supposed to be doing in
> each of the drivers, but the core code is logical and doesn't have a
> confusing overloaded domain = NULL case.
> 
> Something like this as a starting idea of how the drivers could look.
> 
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index b0cde22119875e..143d1abcae2641 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -948,6 +948,20 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
>   		&pagetable);
>   }
>   
> +static struct iommu_domain_ops exynos_private_ops = {
> +	.attach_dev_nofail = &exynos_iommu_detach_device,
> +};
> +
> +static struct iommu_domain exynos_private_domain = {
> +	/*
> +	 * This is private because nobody knew what __sysmmu_disable() does.
> +	 * When someone figures that out this should be made blocking or
> +	 * identity
> +	 */
> +	.type = IOMMU_DOMAN_PRIVATE,
> +	.ops = &exynos_private_ops,
> +};
> +
>   static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>   				   struct device *dev)
>   {
> @@ -1400,11 +1414,11 @@ static const struct iommu_ops exynos_iommu_ops = {
>   	.device_group = generic_device_group,
>   	.probe_device = exynos_iommu_probe_device,
>   	.release_device = exynos_iommu_release_device,
> +	.idle_domain = IS_ENABLED(CONFIG_ARM) ? &exynos_private_domain : NULL,
>   	.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
>   	.of_xlate = exynos_iommu_of_xlate,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
>   		.attach_dev	= exynos_iommu_attach_device,
> -		.detach_dev	= exynos_iommu_detach_device,
>   		.map		= exynos_iommu_map,
>   		.unmap		= exynos_iommu_unmap,
>   		.iova_to_phys	= exynos_iommu_iova_to_phys,
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index de91dd88705bd3..b71a1667f43bd5 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -387,14 +387,13 @@ int iommu_probe_device(struct device *dev)
>   		goto err_release;
>   	}
>   
> -	/*
> -	 * Try to allocate a default domain - needs support from the
> -	 * IOMMU driver. There are still some drivers which don't
> -	 * support default domains, so the return value is not yet
> -	 * checked.
> -	 */
>   	mutex_lock(&group->mutex);
> -	iommu_alloc_default_domain(group, dev);
> +	ret = iommu_alloc_default_domain(group, dev);
> +	if (ret) {
> +		mutex_unlock(&group->mutex);
> +		iommu_group_put(group);
> +		goto err_release;
> +	}
>   
>   	/*
>   	 * If device joined an existing group which has been claimed, don't
> @@ -1645,7 +1644,15 @@ static int iommu_alloc_default_domain(struct iommu_group *group,
>   
>   	type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type;
>   
> -	return iommu_group_alloc_default_domain(dev->bus, group, type);
> +	if (!iommu_group_alloc_default_domain(dev->bus, group, type))
> +		return 0;
> +
> +	/*
> +	 * Driver must support default domains or provide an legacy idle domain
> +	 */
> +	if (WARN_ON(!dev_iommu_ops(dev)->idle_domain))
> +		return -EINVAL;
> +	return 0;
>   }
>   
>   /**
> @@ -2172,17 +2179,22 @@ static int __iommu_group_set_domain(struct iommu_group *group,
>   		return 0;
>   
>   	/*
> -	 * New drivers should support default domains and so the detach_dev() op
> -	 * will never be called. Otherwise the NULL domain represents some
> -	 * platform specific behavior.
> +	 * New drivers should support default domains, otherwise the driver
> +	 * must provide a domain to be attached when the iommu subsystem
> +	 * is not using the device. The purpose of this domain depends
> +	 * on how the iommu driver is operating the dma_ops.
>   	 */
>   	if (!new_domain) {
> -		if (WARN_ON(!group->domain->ops->detach_dev))
> +		struct group_device *grp_dev;
> +		const struct iommu_ops *ops;
> +
> +		grp_dev = list_first_entry(&group->devices, struct group_device,
> +					   list);
> +		ops = dev_iommu_ops(grp_dev->dev);
> +
> +		if (WARN_ON(!ops->idle_domain))
>   			return -EINVAL;
> -		__iommu_group_for_each_dev(group, group->domain,
> -					   iommu_group_do_detach_device);
> -		group->domain = NULL;
> -		return 0;
> +		new_domain = ops->idle_domain;
>   	}
>   
>   	/*
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 46e1347bfa2286..a17069368d70d9 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -91,6 +91,7 @@ struct iommu_domain_geometry {
>   				 __IOMMU_DOMAIN_DMA_API |	\
>   				 __IOMMU_DOMAIN_DMA_FQ)
>   #define IOMMU_DOMAIN_SVA	(__IOMMU_DOMAIN_SVA)
> +#define IOMMU_DOMAN_PRIVATE	(1 << 5)
>   
>   struct iommu_domain {
>   	unsigned type;
> @@ -272,7 +273,18 @@ struct iommu_ops {
>   			     struct iommu_fault_event *evt,
>   			     struct iommu_page_response *msg);
>   
> +	/*
> +	 * driver wants to use dma_ops provided by dma-iommu.c, return
> +	 * the recommended initial IDENTITY/DMA domain.
> +	 */
>   	int (*def_domain_type)(struct device *dev);
> +
> +	/*
> +	 * Driver is not using dma-iommu.c, when the iommu core is not
> +	 * using the device it will attach it to this domain.
> +	 */
> +	struct iommu_domain *idle_domain;
> +
>   	void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid);
>   
>   	const struct iommu_domain_ops *default_domain_ops;
> @@ -315,6 +327,7 @@ struct iommu_ops {
>    * @free: Release the domain after use.
>    */
>   struct iommu_domain_ops {
> +	void (*attach_dev_nofail)(struct iommu_domain *domain, struct device *dev);
>   	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
>   	void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
>   	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
  

On Sat, Feb 18, 2023 at 02:59:16PM +0800, Baolu Lu wrote:

> > arm_iommu doesn't work that way, it allocates and assigns domains so
> > when the platform DMA ops are in control the group->domain is not NULL
> 
> This is what the iommu core assumes, right? Any iommu group should
> always has a domain attached, default domain, blocking domain or driver-
> owned unmanaged domain. The iommu core just switches between different
> domains.

That would be nice, but we still have NULL domains in some cases right
now.
 
> > - which is the opposite of S390's design. Further when arm_iommu asks
> > for a NULL domain it doesn't mean "put it back to platform DMA ops" it
> > really means "park the IOMMU it is not being used anymore"
> 
> This is what identity domain and blocking domains were designed to do,
> right?
>
> If my understanding is right, ARM presumably could implement the
> identity default domain and blocking domain. With that implemented,
> iommu_attach/detac_device() could be removed from drivers and everything
> then could go through the iommu core.

Yes, ideally, but I have no idea what the few reamining drivers do
with their code to properly classify it. So what I showed in the
little sketch was to just mark it DOMAIN_PRIVATE and if someone knows
the right answer they can change it to blocking/identity someday.

In the mean time we can get rid of the NULL domain situation. The core
code would immediately attach either the default or 'idle' domain on
probe and a device will always have a domain attached until release.

I still don't entirely understand how exynox works. In ARM64 mode it
should have default domains, but since it doesn't supply a
def_domain_type() op it only gets the default set of default domains
eg PCI devices.

So on ARM64 some devices don't get default domains, and on ARM32 those
devices use arm_iommu but crashed because of a lack of default domain?

It is confusing. It is harmelss to create a DMA default domain on
ARM32, at worst it wastes a bit of memory. So maybe the fact it
crashes on ARM32 indicates a bug on ARM64 where some devices are not
properly using default domains??

Jason
  

On 2023-02-17 09:47, Lu Baolu wrote:
> iommu_detach_device() attaches the default domain to the device, or if
> default domain is not supported by the IOMMU driver, it calls its
> set_platform_dma_ops callback. If the default domain is supported or
> the IOMMU driver is not iommu-dma aware, iommu_detach_device() is
> actually a noop.
> 
> The 64-bit ARM drivers always support default domain and iommu-dma is
> even not enabled for 32-bit ARM. This turns out that iommu_detach_device()
> is always a noop in arm_iommu_detach_device(). Remove it to avoid dead
> code.

Huh? This call clearly balances the iommu_attach_device() call in 
arm_iommu_attach_device() - it has nothing to do with default domains.

> The bonus is that it also removes a obstacle of arm_iommu_detach_device()
> re-entering the iommu core during release_device. With this removed, the
> iommu core code could be simplified a lot.

That needs to be worked around in those release paths, not by breaking 
the public API. Should probably just be a case of doing as much "detach" 
as necessary directly, then calling arm_iommu_release_mapping(). Just 
beware that arm_teardown_iommu_dma_ops() may or may not have done some 
of it already, depending on whether a driver ever bound to the device.

Thanks,
Robin.

> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   arch/arm/mm/dma-mapping.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 8bc01071474a..dcbc2f4586d4 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1699,7 +1699,6 @@ void arm_iommu_detach_device(struct device *dev)
>   		return;
>   	}
>   
> -	iommu_detach_device(mapping->domain, dev);
>   	kref_put(&mapping->kref, release_iommu_mapping);
>   	to_dma_iommu_mapping(dev) = NULL;
>   	set_dma_ops(dev, NULL);