[PATCH v3] iommu: iommu_group_claim_dma_owner() must always assign a domain

[PATCH v3] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Jason Gunthorpe via iommu]

Once the group enters 'owned' mode it can never be assigned back to the
default_domain or to a NULL domain. It must always be actively assigned to
a current domain. If the caller hasn't provided a domain then the core
must provide an explicit DMA blocking domain that has no DMA map.

Lazily create a group-global blocking DMA domain when
iommu_group_claim_dma_owner is first called and immediately assign the
group to it. This ensures that DMA is immediately fully isolated on all
IOMMU drivers.

If the user attaches/detaches while owned then detach will set the group
back to the blocking domain.

Slightly reorganize the call chains so that
__iommu_group_set_core_domain() is the function that removes any caller
configured domain and sets the domains back a core owned domain with an
appropriate lifetime.

__iommu_group_set_domain() is the worker function that can change the
domain assigned to a group to any target domain, including NULL.

Add comments clarifying how the NULL vs detach_dev vs default_domain works
based on Robin's remarks.

This fixes an oops with VFIO and SMMUv3 because VFIO will call
iommu_detach_group() and then immediately iommu_domain_free(), but
SMMUv3 has no way to know that the domain it is holding a pointer to
has been freed. Now the iommu_detach_group() will assign the blocking
domain and SMMUv3 will no longer hold a stale domain reference.

Fixes: 1ea2a07a532b ("iommu: Add DMA ownership management interfaces")
Reported-by: Qian Cai <quic_qiancai@quicinc.com>
Tested-by: Baolu Lu <baolu.lu@linux.intel.com>
Tested-by: Nicolin Chen <nicolinc@nvidia.com>
Co-developed-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
--

Just minor polishing as discussed

v3:
 - Change names to __iommu_group_set_domain() /
   __iommu_group_set_core_domain()
 - Clarify comments
 - Call __iommu_group_set_domain() directly in
   iommu_group_release_dma_owner() since we know it is always selecting
   the default_domain
v2: https://lore.kernel.org/r/0-v2-f62259511ac0+6-iommu_dma_block_jgg@nvidia.com
 - Remove redundant detach_dev ops check in __iommu_detach_device and
   make the added WARN_ON fail instead
 - Check for blocking_domain in __iommu_attach_group() so VFIO can
   actually attach a new group
 - Update comments and spelling
 - Fix missed change to new_domain in iommu_group_do_detach_device()
v1: https://lore.kernel.org/r/0-v1-6e9d2d0a759d+11b-iommu_dma_block_jgg@nvidia.com

---
 drivers/iommu/iommu.c | 127 ++++++++++++++++++++++++++++++------------
 1 file changed, 91 insertions(+), 36 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0c42ece2585406..0b22e51e90f416 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -44,6 +44,7 @@ struct iommu_group {
 	char *name;
 	int id;
 	struct iommu_domain *default_domain;
+	struct iommu_domain *blocking_domain;
 	struct iommu_domain *domain;
 	struct list_head entry;
 	unsigned int owner_cnt;
@@ -82,8 +83,8 @@ static int __iommu_attach_device(struct iommu_domain *domain,
 				 struct device *dev);
 static int __iommu_attach_group(struct iommu_domain *domain,
 				struct iommu_group *group);
-static void __iommu_detach_group(struct iommu_domain *domain,
-				 struct iommu_group *group);
+static int __iommu_group_set_domain(struct iommu_group *group,
+				    struct iommu_domain *new_domain);
 static int iommu_create_device_direct_mappings(struct iommu_group *group,
 					       struct device *dev);
 static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
@@ -596,6 +597,8 @@ static void iommu_group_release(struct kobject *kobj)
 
 	if (group->default_domain)
 		iommu_domain_free(group->default_domain);
+	if (group->blocking_domain)
+		iommu_domain_free(group->blocking_domain);
 
 	kfree(group->name);
 	kfree(group);
@@ -1907,6 +1910,24 @@ void iommu_domain_free(struct iommu_domain *domain)
 }
 EXPORT_SYMBOL_GPL(iommu_domain_free);
 
+/*
+ * Put the group's domain back to the appropriate core-owned domain - either the
+ * standard kernel-mode DMA configuration or an all-DMA-blocked domain.
+ */
+static void __iommu_group_set_core_domain(struct iommu_group *group)
+{
+	struct iommu_domain *new_domain;
+	int ret;
+
+	if (group->owner)
+		new_domain = group->blocking_domain;
+	else
+		new_domain = group->default_domain;
+
+	ret = __iommu_group_set_domain(group, new_domain);
+	WARN(ret, "iommu driver failed to attach the default/blocking domain");
+}
+
 static int __iommu_attach_device(struct iommu_domain *domain,
 				 struct device *dev)
 {
@@ -1963,9 +1984,6 @@ static void __iommu_detach_device(struct iommu_domain *domain,
 	if (iommu_is_attach_deferred(dev))
 		return;
 
-	if (unlikely(domain->ops->detach_dev == NULL))
-		return;
-
 	domain->ops->detach_dev(domain, dev);
 	trace_detach_device_from_domain(dev);
 }
@@ -1979,12 +1997,10 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
 		return;
 
 	mutex_lock(&group->mutex);
-	if (iommu_group_device_count(group) != 1) {
-		WARN_ON(1);
+	if (WARN_ON(domain != group->domain) ||
+	    WARN_ON(iommu_group_device_count(group) != 1))
 		goto out_unlock;
-	}
-
-	__iommu_detach_group(domain, group);
+	__iommu_group_set_core_domain(group);
 
 out_unlock:
 	mutex_unlock(&group->mutex);
@@ -2040,7 +2056,8 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 {
 	int ret;
 
-	if (group->domain && group->domain != group->default_domain)
+	if (group->domain && group->domain != group->default_domain &&
+	    group->domain != group->blocking_domain)
 		return -EBUSY;
 
 	ret = __iommu_group_for_each_dev(group, domain,
@@ -2072,38 +2089,49 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
 	return 0;
 }
 
-static void __iommu_detach_group(struct iommu_domain *domain,
-				 struct iommu_group *group)
+static int __iommu_group_set_domain(struct iommu_group *group,
+				    struct iommu_domain *new_domain)
 {
 	int ret;
 
+	if (group->domain == new_domain)
+		return 0;
+
 	/*
-	 * If the group has been claimed already, do not re-attach the default
-	 * domain.
+	 * 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.
 	 */
-	if (!group->default_domain || group->owner) {
-		__iommu_group_for_each_dev(group, domain,
+	if (!new_domain) {
+		if (WARN_ON(!group->domain->ops->detach_dev))
+			return -EINVAL;
+		__iommu_group_for_each_dev(group, group->domain,
 					   iommu_group_do_detach_device);
 		group->domain = NULL;
-		return;
+		return 0;
 	}
 
-	if (group->domain == group->default_domain)
-		return;
-
-	/* Detach by re-attaching to the default domain */
-	ret = __iommu_group_for_each_dev(group, group->default_domain,
+	/*
+	 * Changing the domain is done by calling attach_dev() on the new
+	 * domain. This switch does not have to be atomic and DMA can be
+	 * discarded during the transition. DMA must only be able to access
+	 * either new_domain or group->domain, never something else.
+	 *
+	 * Note that this is called in error unwind paths, attaching to a
+	 * domain that has already been attached cannot fail.
+	 */
+	ret = __iommu_group_for_each_dev(group, new_domain,
 					 iommu_group_do_attach_device);
-	if (ret != 0)
-		WARN_ON(1);
-	else
-		group->domain = group->default_domain;
+	if (ret)
+		return ret;
+	group->domain = new_domain;
+	return 0;
 }
 
 void iommu_detach_group(struct iommu_domain *domain, struct iommu_group *group)
 {
 	mutex_lock(&group->mutex);
-	__iommu_detach_group(domain, group);
+	__iommu_group_set_core_domain(group);
 	mutex_unlock(&group->mutex);
 }
 EXPORT_SYMBOL_GPL(iommu_detach_group);
@@ -3088,6 +3116,29 @@ void iommu_device_unuse_default_domain(struct device *dev)
 	iommu_group_put(group);
 }
 
+static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
+{
+	struct group_device *dev =
+		list_first_entry(&group->devices, struct group_device, list);
+
+	if (group->blocking_domain)
+		return 0;
+
+	group->blocking_domain =
+		__iommu_domain_alloc(dev->dev->bus, IOMMU_DOMAIN_BLOCKED);
+	if (!group->blocking_domain) {
+		/*
+		 * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
+		 * create an empty domain instead.
+		 */
+		group->blocking_domain = __iommu_domain_alloc(
+			dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
+		if (!group->blocking_domain)
+			return -EINVAL;
+	}
+	return 0;
+}
+
 /**
  * iommu_group_claim_dma_owner() - Set DMA ownership of a group
  * @group: The group.
@@ -3111,9 +3162,14 @@ int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner)
 			goto unlock_out;
 		}
 
+		ret = __iommu_group_alloc_blocking_domain(group);
+		if (ret)
+			goto unlock_out;
+
+		ret = __iommu_group_set_domain(group, group->blocking_domain);
+		if (ret)
+			goto unlock_out;
 		group->owner = owner;
-		if (group->domain)
-			__iommu_detach_group(group->domain, group);
 	}
 
 	group->owner_cnt++;
@@ -3132,18 +3188,17 @@ EXPORT_SYMBOL_GPL(iommu_group_claim_dma_owner);
  */
 void iommu_group_release_dma_owner(struct iommu_group *group)
 {
+	int ret;
+
 	mutex_lock(&group->mutex);
 	if (WARN_ON(!group->owner_cnt || !group->owner))
 		goto unlock_out;
 
 	group->owner_cnt = 0;
-	/*
-	 * The UNMANAGED domain should be detached before all USER
-	 * owners have been released.
-	 */
-	if (!WARN_ON(group->domain) && group->default_domain)
-		__iommu_attach_group(group->default_domain, group);
 	group->owner = NULL;
+	ret = __iommu_group_set_domain(group, group->default_domain);
+	WARN(ret, "iommu driver failed to attach the default domain");
+
 unlock_out:
 	mutex_unlock(&group->mutex);
 }

base-commit: da844db4722bdd333142b40f0e414e2aedc2a4c0
-- 
2.36.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Robin Murphy]

On 2022-05-09 17:19, Jason Gunthorpe wrote:
> Once the group enters 'owned' mode it can never be assigned back to the
> default_domain or to a NULL domain. It must always be actively assigned to
> a current domain. If the caller hasn't provided a domain then the core
> must provide an explicit DMA blocking domain that has no DMA map.
> 
> Lazily create a group-global blocking DMA domain when
> iommu_group_claim_dma_owner is first called and immediately assign the
> group to it. This ensures that DMA is immediately fully isolated on all
> IOMMU drivers.
> 
> If the user attaches/detaches while owned then detach will set the group
> back to the blocking domain.
> 
> Slightly reorganize the call chains so that
> __iommu_group_set_core_domain() is the function that removes any caller
> configured domain and sets the domains back a core owned domain with an
> appropriate lifetime.
> 
> __iommu_group_set_domain() is the worker function that can change the
> domain assigned to a group to any target domain, including NULL.
> 
> Add comments clarifying how the NULL vs detach_dev vs default_domain works
> based on Robin's remarks.
> 
> This fixes an oops with VFIO and SMMUv3 because VFIO will call
> iommu_detach_group() and then immediately iommu_domain_free(), but
> SMMUv3 has no way to know that the domain it is holding a pointer to
> has been freed. Now the iommu_detach_group() will assign the blocking
> domain and SMMUv3 will no longer hold a stale domain reference.

Thanks Jason, from my PoV this looks great now - I still think it feels 
too silly to give a formal review tag for a patch with my own sign-off, 
so this is just my ephemeral "let's get this branch back in -next ASAP 
and hope nothing else shakes loose" :)

Cheers,
Robin.

> Fixes: 1ea2a07a532b ("iommu: Add DMA ownership management interfaces")
> Reported-by: Qian Cai <quic_qiancai@quicinc.com>
> Tested-by: Baolu Lu <baolu.lu@linux.intel.com>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Co-developed-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> --
> 
> Just minor polishing as discussed
> 
> v3:
>   - Change names to __iommu_group_set_domain() /
>     __iommu_group_set_core_domain()
>   - Clarify comments
>   - Call __iommu_group_set_domain() directly in
>     iommu_group_release_dma_owner() since we know it is always selecting
>     the default_domain
> v2: https://lore.kernel.org/r/0-v2-f62259511ac0+6-iommu_dma_block_jgg@nvidia.com
>   - Remove redundant detach_dev ops check in __iommu_detach_device and
>     make the added WARN_ON fail instead
>   - Check for blocking_domain in __iommu_attach_group() so VFIO can
>     actually attach a new group
>   - Update comments and spelling
>   - Fix missed change to new_domain in iommu_group_do_detach_device()
> v1: https://lore.kernel.org/r/0-v1-6e9d2d0a759d+11b-iommu_dma_block_jgg@nvidia.com
> 
> ---
>   drivers/iommu/iommu.c | 127 ++++++++++++++++++++++++++++++------------
>   1 file changed, 91 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 0c42ece2585406..0b22e51e90f416 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -44,6 +44,7 @@ struct iommu_group {
>   	char *name;
>   	int id;
>   	struct iommu_domain *default_domain;
> +	struct iommu_domain *blocking_domain;
>   	struct iommu_domain *domain;
>   	struct list_head entry;
>   	unsigned int owner_cnt;
> @@ -82,8 +83,8 @@ static int __iommu_attach_device(struct iommu_domain *domain,
>   				 struct device *dev);
>   static int __iommu_attach_group(struct iommu_domain *domain,
>   				struct iommu_group *group);
> -static void __iommu_detach_group(struct iommu_domain *domain,
> -				 struct iommu_group *group);
> +static int __iommu_group_set_domain(struct iommu_group *group,
> +				    struct iommu_domain *new_domain);
>   static int iommu_create_device_direct_mappings(struct iommu_group *group,
>   					       struct device *dev);
>   static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> @@ -596,6 +597,8 @@ static void iommu_group_release(struct kobject *kobj)
>   
>   	if (group->default_domain)
>   		iommu_domain_free(group->default_domain);
> +	if (group->blocking_domain)
> +		iommu_domain_free(group->blocking_domain);
>   
>   	kfree(group->name);
>   	kfree(group);
> @@ -1907,6 +1910,24 @@ void iommu_domain_free(struct iommu_domain *domain)
>   }
>   EXPORT_SYMBOL_GPL(iommu_domain_free);
>   
> +/*
> + * Put the group's domain back to the appropriate core-owned domain - either the
> + * standard kernel-mode DMA configuration or an all-DMA-blocked domain.
> + */
> +static void __iommu_group_set_core_domain(struct iommu_group *group)
> +{
> +	struct iommu_domain *new_domain;
> +	int ret;
> +
> +	if (group->owner)
> +		new_domain = group->blocking_domain;
> +	else
> +		new_domain = group->default_domain;
> +
> +	ret = __iommu_group_set_domain(group, new_domain);
> +	WARN(ret, "iommu driver failed to attach the default/blocking domain");
> +}
> +
>   static int __iommu_attach_device(struct iommu_domain *domain,
>   				 struct device *dev)
>   {
> @@ -1963,9 +1984,6 @@ static void __iommu_detach_device(struct iommu_domain *domain,
>   	if (iommu_is_attach_deferred(dev))
>   		return;
>   
> -	if (unlikely(domain->ops->detach_dev == NULL))
> -		return;
> -
>   	domain->ops->detach_dev(domain, dev);
>   	trace_detach_device_from_domain(dev);
>   }
> @@ -1979,12 +1997,10 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
>   		return;
>   
>   	mutex_lock(&group->mutex);
> -	if (iommu_group_device_count(group) != 1) {
> -		WARN_ON(1);
> +	if (WARN_ON(domain != group->domain) ||
> +	    WARN_ON(iommu_group_device_count(group) != 1))
>   		goto out_unlock;
> -	}
> -
> -	__iommu_detach_group(domain, group);
> +	__iommu_group_set_core_domain(group);
>   
>   out_unlock:
>   	mutex_unlock(&group->mutex);
> @@ -2040,7 +2056,8 @@ static int __iommu_attach_group(struct iommu_domain *domain,
>   {
>   	int ret;
>   
> -	if (group->domain && group->domain != group->default_domain)
> +	if (group->domain && group->domain != group->default_domain &&
> +	    group->domain != group->blocking_domain)
>   		return -EBUSY;
>   
>   	ret = __iommu_group_for_each_dev(group, domain,
> @@ -2072,38 +2089,49 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
>   	return 0;
>   }
>   
> -static void __iommu_detach_group(struct iommu_domain *domain,
> -				 struct iommu_group *group)
> +static int __iommu_group_set_domain(struct iommu_group *group,
> +				    struct iommu_domain *new_domain)
>   {
>   	int ret;
>   
> +	if (group->domain == new_domain)
> +		return 0;
> +
>   	/*
> -	 * If the group has been claimed already, do not re-attach the default
> -	 * domain.
> +	 * 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.
>   	 */
> -	if (!group->default_domain || group->owner) {
> -		__iommu_group_for_each_dev(group, domain,
> +	if (!new_domain) {
> +		if (WARN_ON(!group->domain->ops->detach_dev))
> +			return -EINVAL;
> +		__iommu_group_for_each_dev(group, group->domain,
>   					   iommu_group_do_detach_device);
>   		group->domain = NULL;
> -		return;
> +		return 0;
>   	}
>   
> -	if (group->domain == group->default_domain)
> -		return;
> -
> -	/* Detach by re-attaching to the default domain */
> -	ret = __iommu_group_for_each_dev(group, group->default_domain,
> +	/*
> +	 * Changing the domain is done by calling attach_dev() on the new
> +	 * domain. This switch does not have to be atomic and DMA can be
> +	 * discarded during the transition. DMA must only be able to access
> +	 * either new_domain or group->domain, never something else.
> +	 *
> +	 * Note that this is called in error unwind paths, attaching to a
> +	 * domain that has already been attached cannot fail.
> +	 */
> +	ret = __iommu_group_for_each_dev(group, new_domain,
>   					 iommu_group_do_attach_device);
> -	if (ret != 0)
> -		WARN_ON(1);
> -	else
> -		group->domain = group->default_domain;
> +	if (ret)
> +		return ret;
> +	group->domain = new_domain;
> +	return 0;
>   }
>   
>   void iommu_detach_group(struct iommu_domain *domain, struct iommu_group *group)
>   {
>   	mutex_lock(&group->mutex);
> -	__iommu_detach_group(domain, group);
> +	__iommu_group_set_core_domain(group);
>   	mutex_unlock(&group->mutex);
>   }
>   EXPORT_SYMBOL_GPL(iommu_detach_group);
> @@ -3088,6 +3116,29 @@ void iommu_device_unuse_default_domain(struct device *dev)
>   	iommu_group_put(group);
>   }
>   
> +static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
> +{
> +	struct group_device *dev =
> +		list_first_entry(&group->devices, struct group_device, list);
> +
> +	if (group->blocking_domain)
> +		return 0;
> +
> +	group->blocking_domain =
> +		__iommu_domain_alloc(dev->dev->bus, IOMMU_DOMAIN_BLOCKED);
> +	if (!group->blocking_domain) {
> +		/*
> +		 * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
> +		 * create an empty domain instead.
> +		 */
> +		group->blocking_domain = __iommu_domain_alloc(
> +			dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
> +		if (!group->blocking_domain)
> +			return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>   /**
>    * iommu_group_claim_dma_owner() - Set DMA ownership of a group
>    * @group: The group.
> @@ -3111,9 +3162,14 @@ int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner)
>   			goto unlock_out;
>   		}
>   
> +		ret = __iommu_group_alloc_blocking_domain(group);
> +		if (ret)
> +			goto unlock_out;
> +
> +		ret = __iommu_group_set_domain(group, group->blocking_domain);
> +		if (ret)
> +			goto unlock_out;
>   		group->owner = owner;
> -		if (group->domain)
> -			__iommu_detach_group(group->domain, group);
>   	}
>   
>   	group->owner_cnt++;
> @@ -3132,18 +3188,17 @@ EXPORT_SYMBOL_GPL(iommu_group_claim_dma_owner);
>    */
>   void iommu_group_release_dma_owner(struct iommu_group *group)
>   {
> +	int ret;
> +
>   	mutex_lock(&group->mutex);
>   	if (WARN_ON(!group->owner_cnt || !group->owner))
>   		goto unlock_out;
>   
>   	group->owner_cnt = 0;
> -	/*
> -	 * The UNMANAGED domain should be detached before all USER
> -	 * owners have been released.
> -	 */
> -	if (!WARN_ON(group->domain) && group->default_domain)
> -		__iommu_attach_group(group->default_domain, group);
>   	group->owner = NULL;
> +	ret = __iommu_group_set_domain(group, group->default_domain);
> +	WARN(ret, "iommu driver failed to attach the default domain");
> +
>   unlock_out:
>   	mutex_unlock(&group->mutex);
>   }
> 
> base-commit: da844db4722bdd333142b40f0e414e2aedc2a4c0
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v3] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, May 10, 2022 12:19 AM
> 
> Once the group enters 'owned' mode it can never be assigned back to the
> default_domain or to a NULL domain. It must always be actively assigned to
> a current domain. If the caller hasn't provided a domain then the core
> must provide an explicit DMA blocking domain that has no DMA map.
> 
> Lazily create a group-global blocking DMA domain when
> iommu_group_claim_dma_owner is first called and immediately assign the
> group to it. This ensures that DMA is immediately fully isolated on all
> IOMMU drivers.
> 
> If the user attaches/detaches while owned then detach will set the group
> back to the blocking domain.
> 
> Slightly reorganize the call chains so that
> __iommu_group_set_core_domain() is the function that removes any caller
> configured domain and sets the domains back a core owned domain with an
> appropriate lifetime.
> 
> __iommu_group_set_domain() is the worker function that can change the
> domain assigned to a group to any target domain, including NULL.
> 
> Add comments clarifying how the NULL vs detach_dev vs default_domain
> works
> based on Robin's remarks.
> 
> This fixes an oops with VFIO and SMMUv3 because VFIO will call
> iommu_detach_group() and then immediately iommu_domain_free(), but
> SMMUv3 has no way to know that the domain it is holding a pointer to
> has been freed. Now the iommu_detach_group() will assign the blocking
> domain and SMMUv3 will no longer hold a stale domain reference.
> 
> Fixes: 1ea2a07a532b ("iommu: Add DMA ownership management
> interfaces")
> Reported-by: Qian Cai <quic_qiancai@quicinc.com>
> Tested-by: Baolu Lu <baolu.lu@linux.intel.com>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Co-developed-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> --
> 
> Just minor polishing as discussed
> 
> v3:
>  - Change names to __iommu_group_set_domain() /
>    __iommu_group_set_core_domain()
>  - Clarify comments
>  - Call __iommu_group_set_domain() directly in
>    iommu_group_release_dma_owner() since we know it is always selecting
>    the default_domain
> v2: https://lore.kernel.org/r/0-v2-f62259511ac0+6-
> iommu_dma_block_jgg@nvidia.com
>  - Remove redundant detach_dev ops check in __iommu_detach_device and
>    make the added WARN_ON fail instead
>  - Check for blocking_domain in __iommu_attach_group() so VFIO can
>    actually attach a new group
>  - Update comments and spelling
>  - Fix missed change to new_domain in iommu_group_do_detach_device()
> v1: https://lore.kernel.org/r/0-v1-6e9d2d0a759d+11b-
> iommu_dma_block_jgg@nvidia.com
> 
> ---
>  drivers/iommu/iommu.c | 127 ++++++++++++++++++++++++++++++------------
>  1 file changed, 91 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 0c42ece2585406..0b22e51e90f416 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -44,6 +44,7 @@ struct iommu_group {
>  	char *name;
>  	int id;
>  	struct iommu_domain *default_domain;
> +	struct iommu_domain *blocking_domain;
>  	struct iommu_domain *domain;
>  	struct list_head entry;
>  	unsigned int owner_cnt;
> @@ -82,8 +83,8 @@ static int __iommu_attach_device(struct
> iommu_domain *domain,
>  				 struct device *dev);
>  static int __iommu_attach_group(struct iommu_domain *domain,
>  				struct iommu_group *group);
> -static void __iommu_detach_group(struct iommu_domain *domain,
> -				 struct iommu_group *group);
> +static int __iommu_group_set_domain(struct iommu_group *group,
> +				    struct iommu_domain *new_domain);
>  static int iommu_create_device_direct_mappings(struct iommu_group
> *group,
>  					       struct device *dev);
>  static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> @@ -596,6 +597,8 @@ static void iommu_group_release(struct kobject
> *kobj)
> 
>  	if (group->default_domain)
>  		iommu_domain_free(group->default_domain);
> +	if (group->blocking_domain)
> +		iommu_domain_free(group->blocking_domain);
> 
>  	kfree(group->name);
>  	kfree(group);
> @@ -1907,6 +1910,24 @@ void iommu_domain_free(struct iommu_domain
> *domain)
>  }
>  EXPORT_SYMBOL_GPL(iommu_domain_free);
> 
> +/*
> + * Put the group's domain back to the appropriate core-owned domain -
> either the
> + * standard kernel-mode DMA configuration or an all-DMA-blocked domain.
> + */
> +static void __iommu_group_set_core_domain(struct iommu_group *group)
> +{
> +	struct iommu_domain *new_domain;
> +	int ret;
> +
> +	if (group->owner)
> +		new_domain = group->blocking_domain;
> +	else
> +		new_domain = group->default_domain;
> +
> +	ret = __iommu_group_set_domain(group, new_domain);
> +	WARN(ret, "iommu driver failed to attach the default/blocking
> domain");
> +}
> +
>  static int __iommu_attach_device(struct iommu_domain *domain,
>  				 struct device *dev)
>  {
> @@ -1963,9 +1984,6 @@ static void __iommu_detach_device(struct
> iommu_domain *domain,
>  	if (iommu_is_attach_deferred(dev))
>  		return;
> 
> -	if (unlikely(domain->ops->detach_dev == NULL))
> -		return;
> -
>  	domain->ops->detach_dev(domain, dev);
>  	trace_detach_device_from_domain(dev);
>  }
> @@ -1979,12 +1997,10 @@ void iommu_detach_device(struct
> iommu_domain *domain, struct device *dev)
>  		return;
> 
>  	mutex_lock(&group->mutex);
> -	if (iommu_group_device_count(group) != 1) {
> -		WARN_ON(1);
> +	if (WARN_ON(domain != group->domain) ||
> +	    WARN_ON(iommu_group_device_count(group) != 1))
>  		goto out_unlock;
> -	}
> -
> -	__iommu_detach_group(domain, group);
> +	__iommu_group_set_core_domain(group);
> 
>  out_unlock:
>  	mutex_unlock(&group->mutex);
> @@ -2040,7 +2056,8 @@ static int __iommu_attach_group(struct
> iommu_domain *domain,
>  {
>  	int ret;
> 
> -	if (group->domain && group->domain != group->default_domain)
> +	if (group->domain && group->domain != group->default_domain &&
> +	    group->domain != group->blocking_domain)
>  		return -EBUSY;
> 
>  	ret = __iommu_group_for_each_dev(group, domain,
> @@ -2072,38 +2089,49 @@ static int
> iommu_group_do_detach_device(struct device *dev, void *data)
>  	return 0;
>  }
> 
> -static void __iommu_detach_group(struct iommu_domain *domain,
> -				 struct iommu_group *group)
> +static int __iommu_group_set_domain(struct iommu_group *group,
> +				    struct iommu_domain *new_domain)
>  {
>  	int ret;
> 
> +	if (group->domain == new_domain)
> +		return 0;
> +
>  	/*
> -	 * If the group has been claimed already, do not re-attach the default
> -	 * domain.
> +	 * 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.
>  	 */
> -	if (!group->default_domain || group->owner) {
> -		__iommu_group_for_each_dev(group, domain,
> +	if (!new_domain) {
> +		if (WARN_ON(!group->domain->ops->detach_dev))
> +			return -EINVAL;
> +		__iommu_group_for_each_dev(group, group->domain,
>  					   iommu_group_do_detach_device);
>  		group->domain = NULL;
> -		return;
> +		return 0;
>  	}
> 
> -	if (group->domain == group->default_domain)
> -		return;
> -
> -	/* Detach by re-attaching to the default domain */
> -	ret = __iommu_group_for_each_dev(group, group->default_domain,
> +	/*
> +	 * Changing the domain is done by calling attach_dev() on the new
> +	 * domain. This switch does not have to be atomic and DMA can be
> +	 * discarded during the transition. DMA must only be able to access
> +	 * either new_domain or group->domain, never something else.
> +	 *
> +	 * Note that this is called in error unwind paths, attaching to a
> +	 * domain that has already been attached cannot fail.
> +	 */
> +	ret = __iommu_group_for_each_dev(group, new_domain,
>  					 iommu_group_do_attach_device);
> -	if (ret != 0)
> -		WARN_ON(1);
> -	else
> -		group->domain = group->default_domain;
> +	if (ret)
> +		return ret;
> +	group->domain = new_domain;
> +	return 0;
>  }
> 
>  void iommu_detach_group(struct iommu_domain *domain, struct
> iommu_group *group)
>  {
>  	mutex_lock(&group->mutex);
> -	__iommu_detach_group(domain, group);
> +	__iommu_group_set_core_domain(group);
>  	mutex_unlock(&group->mutex);
>  }
>  EXPORT_SYMBOL_GPL(iommu_detach_group);
> @@ -3088,6 +3116,29 @@ void
> iommu_device_unuse_default_domain(struct device *dev)
>  	iommu_group_put(group);
>  }
> 
> +static int __iommu_group_alloc_blocking_domain(struct iommu_group
> *group)
> +{
> +	struct group_device *dev =
> +		list_first_entry(&group->devices, struct group_device, list);
> +
> +	if (group->blocking_domain)
> +		return 0;
> +
> +	group->blocking_domain =
> +		__iommu_domain_alloc(dev->dev->bus,
> IOMMU_DOMAIN_BLOCKED);
> +	if (!group->blocking_domain) {
> +		/*
> +		 * For drivers that do not yet understand
> IOMMU_DOMAIN_BLOCKED
> +		 * create an empty domain instead.
> +		 */
> +		group->blocking_domain = __iommu_domain_alloc(
> +			dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
> +		if (!group->blocking_domain)
> +			return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  /**
>   * iommu_group_claim_dma_owner() - Set DMA ownership of a group
>   * @group: The group.
> @@ -3111,9 +3162,14 @@ int iommu_group_claim_dma_owner(struct
> iommu_group *group, void *owner)
>  			goto unlock_out;
>  		}
> 
> +		ret = __iommu_group_alloc_blocking_domain(group);
> +		if (ret)
> +			goto unlock_out;
> +
> +		ret = __iommu_group_set_domain(group, group-
> >blocking_domain);
> +		if (ret)
> +			goto unlock_out;
>  		group->owner = owner;
> -		if (group->domain)
> -			__iommu_detach_group(group->domain, group);
>  	}
> 
>  	group->owner_cnt++;
> @@ -3132,18 +3188,17 @@
> EXPORT_SYMBOL_GPL(iommu_group_claim_dma_owner);
>   */
>  void iommu_group_release_dma_owner(struct iommu_group *group)
>  {
> +	int ret;
> +
>  	mutex_lock(&group->mutex);
>  	if (WARN_ON(!group->owner_cnt || !group->owner))
>  		goto unlock_out;
> 
>  	group->owner_cnt = 0;
> -	/*
> -	 * The UNMANAGED domain should be detached before all USER
> -	 * owners have been released.
> -	 */
> -	if (!WARN_ON(group->domain) && group->default_domain)
> -		__iommu_attach_group(group->default_domain, group);
>  	group->owner = NULL;
> +	ret = __iommu_group_set_domain(group, group->default_domain);
> +	WARN(ret, "iommu driver failed to attach the default domain");
> +
>  unlock_out:
>  	mutex_unlock(&group->mutex);
>  }
> 
> base-commit: da844db4722bdd333142b40f0e414e2aedc2a4c0
> --
> 2.36.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Joerg Roedel]

On Mon, May 09, 2022 at 01:19:19PM -0300, Jason Gunthorpe wrote:
>  drivers/iommu/iommu.c | 127 ++++++++++++++++++++++++++++++------------
>  1 file changed, 91 insertions(+), 36 deletions(-)

Applied, thanks. Will back-merge the branch into next now.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Eric Farman]

On Mon, 2022-05-09 at 13:19 -0300, Jason Gunthorpe via iommu wrote:
> Once the group enters 'owned' mode it can never be assigned back to
> the
> default_domain or to a NULL domain. It must always be actively
> assigned to
> a current domain. If the caller hasn't provided a domain then the
> core
> must provide an explicit DMA blocking domain that has no DMA map.
> 
> Lazily create a group-global blocking DMA domain when
> iommu_group_claim_dma_owner is first called and immediately assign
> the
> group to it. This ensures that DMA is immediately fully isolated on
> all
> IOMMU drivers.
> 
> If the user attaches/detaches while owned then detach will set the
> group
> back to the blocking domain.
> 
> Slightly reorganize the call chains so that
> __iommu_group_set_core_domain() is the function that removes any
> caller
> configured domain and sets the domains back a core owned domain with
> an
> appropriate lifetime.
> 
> __iommu_group_set_domain() is the worker function that can change the
> domain assigned to a group to any target domain, including NULL.
> 
> Add comments clarifying how the NULL vs detach_dev vs default_domain
> works
> based on Robin's remarks.
> 
> This fixes an oops with VFIO and SMMUv3 because VFIO will call
> iommu_detach_group() and then immediately iommu_domain_free(), but
> SMMUv3 has no way to know that the domain it is holding a pointer to
> has been freed. Now the iommu_detach_group() will assign the blocking
> domain and SMMUv3 will no longer hold a stale domain reference.
> 
> Fixes: 1ea2a07a532b ("iommu: Add DMA ownership management
> interfaces")
> Reported-by: Qian Cai <quic_qiancai@quicinc.com>
> Tested-by: Baolu Lu <baolu.lu@linux.intel.com>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Co-developed-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> --
> 
> Just minor polishing as discussed
> 
> v3:
>  - Change names to __iommu_group_set_domain() /
>    __iommu_group_set_core_domain()
>  - Clarify comments
>  - Call __iommu_group_set_domain() directly in
>    iommu_group_release_dma_owner() since we know it is always
> selecting
>    the default_domain
> v2: 
> https://lore.kernel.org/r/0-v2-f62259511ac0+6-iommu_dma_block_jgg@nvidia.com
>  - Remove redundant detach_dev ops check in __iommu_detach_device and
>    make the added WARN_ON fail instead
>  - Check for blocking_domain in __iommu_attach_group() so VFIO can
>    actually attach a new group
>  - Update comments and spelling
>  - Fix missed change to new_domain in iommu_group_do_detach_device()
> v1: 
> https://lore.kernel.org/r/0-v1-6e9d2d0a759d+11b-iommu_dma_block_jgg@nvidia.com
> 
> ---
>  drivers/iommu/iommu.c | 127 ++++++++++++++++++++++++++++++--------
> ----
>  1 file changed, 91 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 0c42ece2585406..0b22e51e90f416 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -44,6 +44,7 @@ struct iommu_group {
>  	char *name;
>  	int id;
>  	struct iommu_domain *default_domain;
> +	struct iommu_domain *blocking_domain;
>  	struct iommu_domain *domain;
>  	struct list_head entry;
>  	unsigned int owner_cnt;
> @@ -82,8 +83,8 @@ static int __iommu_attach_device(struct
> iommu_domain *domain,
>  				 struct device *dev);
>  static int __iommu_attach_group(struct iommu_domain *domain,
>  				struct iommu_group *group);
> -static void __iommu_detach_group(struct iommu_domain *domain,
> -				 struct iommu_group *group);
> +static int __iommu_group_set_domain(struct iommu_group *group,
> +				    struct iommu_domain *new_domain);
>  static int iommu_create_device_direct_mappings(struct iommu_group
> *group,
>  					       struct device *dev);
>  static struct iommu_group *iommu_group_get_for_dev(struct device
> *dev);
> @@ -596,6 +597,8 @@ static void iommu_group_release(struct kobject
> *kobj)
>  
>  	if (group->default_domain)
>  		iommu_domain_free(group->default_domain);
> +	if (group->blocking_domain)
> +		iommu_domain_free(group->blocking_domain);
>  
>  	kfree(group->name);
>  	kfree(group);
> @@ -1907,6 +1910,24 @@ void iommu_domain_free(struct iommu_domain
> *domain)
>  }
>  EXPORT_SYMBOL_GPL(iommu_domain_free);
>  
> +/*
> + * Put the group's domain back to the appropriate core-owned domain
> - either the
> + * standard kernel-mode DMA configuration or an all-DMA-blocked
> domain.
> + */
> +static void __iommu_group_set_core_domain(struct iommu_group *group)
> +{
> +	struct iommu_domain *new_domain;
> +	int ret;
> +
> +	if (group->owner)
> +		new_domain = group->blocking_domain;
> +	else
> +		new_domain = group->default_domain;
> +
> +	ret = __iommu_group_set_domain(group, new_domain);
> +	WARN(ret, "iommu driver failed to attach the default/blocking
> domain");
> +}
> +
>  static int __iommu_attach_device(struct iommu_domain *domain,
>  				 struct device *dev)
>  {
> @@ -1963,9 +1984,6 @@ static void __iommu_detach_device(struct
> iommu_domain *domain,
>  	if (iommu_is_attach_deferred(dev))
>  		return;
>  
> -	if (unlikely(domain->ops->detach_dev == NULL))
> -		return;
> -
>  	domain->ops->detach_dev(domain, dev);
>  	trace_detach_device_from_domain(dev);
>  }
> @@ -1979,12 +1997,10 @@ void iommu_detach_device(struct iommu_domain
> *domain, struct device *dev)
>  		return;
>  
>  	mutex_lock(&group->mutex);
> -	if (iommu_group_device_count(group) != 1) {
> -		WARN_ON(1);
> +	if (WARN_ON(domain != group->domain) ||
> +	    WARN_ON(iommu_group_device_count(group) != 1))
>  		goto out_unlock;
> -	}
> -
> -	__iommu_detach_group(domain, group);
> +	__iommu_group_set_core_domain(group);
>  
>  out_unlock:
>  	mutex_unlock(&group->mutex);
> @@ -2040,7 +2056,8 @@ static int __iommu_attach_group(struct
> iommu_domain *domain,
>  {
>  	int ret;
>  
> -	if (group->domain && group->domain != group->default_domain)
> +	if (group->domain && group->domain != group->default_domain &&
> +	    group->domain != group->blocking_domain)
>  		return -EBUSY;
>  
>  	ret = __iommu_group_for_each_dev(group, domain,
> @@ -2072,38 +2089,49 @@ static int
> iommu_group_do_detach_device(struct device *dev, void *data)
>  	return 0;
>  }
>  
> -static void __iommu_detach_group(struct iommu_domain *domain,
> -				 struct iommu_group *group)
> +static int __iommu_group_set_domain(struct iommu_group *group,
> +				    struct iommu_domain *new_domain)
>  {
>  	int ret;
>  
> +	if (group->domain == new_domain)
> +		return 0;
> +
>  	/*
> -	 * If the group has been claimed already, do not re-attach the
> default
> -	 * domain.
> +	 * 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.
>  	 */
> -	if (!group->default_domain || group->owner) {
> -		__iommu_group_for_each_dev(group, domain,
> +	if (!new_domain) {
> +		if (WARN_ON(!group->domain->ops->detach_dev))
> +			return -EINVAL;
> +		__iommu_group_for_each_dev(group, group->domain,
>  					   iommu_group_do_detach_device
> );
>  		group->domain = NULL;
> -		return;
> +		return 0;
>  	}
>  
> -	if (group->domain == group->default_domain)
> -		return;
> -
> -	/* Detach by re-attaching to the default domain */
> -	ret = __iommu_group_for_each_dev(group, group->default_domain,
> +	/*
> +	 * Changing the domain is done by calling attach_dev() on the
> new
> +	 * domain. This switch does not have to be atomic and DMA can
> be
> +	 * discarded during the transition. DMA must only be able to
> access
> +	 * either new_domain or group->domain, never something else.
> +	 *
> +	 * Note that this is called in error unwind paths, attaching to
> a
> +	 * domain that has already been attached cannot fail.
> +	 */
> +	ret = __iommu_group_for_each_dev(group, new_domain,
>  					 iommu_group_do_attach_device);
> -	if (ret != 0)
> -		WARN_ON(1);
> -	else
> -		group->domain = group->default_domain;
> +	if (ret)
> +		return ret;
> +	group->domain = new_domain;
> +	return 0;
>  }
>  
>  void iommu_detach_group(struct iommu_domain *domain, struct
> iommu_group *group)
>  {
>  	mutex_lock(&group->mutex);
> -	__iommu_detach_group(domain, group);
> +	__iommu_group_set_core_domain(group);
>  	mutex_unlock(&group->mutex);
>  }
>  EXPORT_SYMBOL_GPL(iommu_detach_group);
> @@ -3088,6 +3116,29 @@ void iommu_device_unuse_default_domain(struct
> device *dev)
>  	iommu_group_put(group);
>  }
>  
> +static int __iommu_group_alloc_blocking_domain(struct iommu_group
> *group)
> +{
> +	struct group_device *dev =
> +		list_first_entry(&group->devices, struct group_device,
> list);
> +
> +	if (group->blocking_domain)
> +		return 0;
> +
> +	group->blocking_domain =
> +		__iommu_domain_alloc(dev->dev->bus,
> IOMMU_DOMAIN_BLOCKED);
> +	if (!group->blocking_domain) {
> +		/*
> +		 * For drivers that do not yet understand
> IOMMU_DOMAIN_BLOCKED
> +		 * create an empty domain instead.
> +		 */
> +		group->blocking_domain = __iommu_domain_alloc(
> +			dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
> +		if (!group->blocking_domain)
> +			return -EINVAL;

Hi Jason,

I got a heads up from Matt about the s390 KVM vfio- variants failing on
linux-next.

For vfio-ap and vfio-ccw, they fail on the above error. Both calls to
__iommu_domain_alloc fail because while dev->dev->bus is non-NULL (it
points to the mdev bus_type registered in mdev_init()), the bus-
>iommu_ops pointer is NULL. Which makes sense; the iommu_group is vfio-
noiommu, via vfio_register_emulated_iommu_dev(), and mdev didn't
establish an iommu_ops for its bus.

The caller of this, iommu_group_claim_dma_owner(), was added to
vfio_group_set_container() by commit 70693f470848 ("vfio: Set DMA
ownership for VFIO devices") [1] ... But that's as far as I got without
making some probably incorrect decisions. Do you have any thoughts
here?

Thanks,
Eric

[1] 
https://lore.kernel.org/r/20220418005000.897664-8-baolu.lu@linux.intel.com

> +	}
> +	return 0;
> +}
> +
>  /**
>   * iommu_group_claim_dma_owner() - Set DMA ownership of a group
>   * @group: The group.
> @@ -3111,9 +3162,14 @@ int iommu_group_claim_dma_owner(struct
> iommu_group *group, void *owner)
>  			goto unlock_out;
>  		}
>  
> +		ret = __iommu_group_alloc_blocking_domain(group);
> +		if (ret)
> +			goto unlock_out;
> +
> +		ret = __iommu_group_set_domain(group, group-
> >blocking_domain);
> +		if (ret)
> +			goto unlock_out;
>  		group->owner = owner;
> -		if (group->domain)
> -			__iommu_detach_group(group->domain, group);
>  	}
>  
>  	group->owner_cnt++;
> @@ -3132,18 +3188,17 @@
> EXPORT_SYMBOL_GPL(iommu_group_claim_dma_owner);
>   */
>  void iommu_group_release_dma_owner(struct iommu_group *group)
>  {
> +	int ret;
> +
>  	mutex_lock(&group->mutex);
>  	if (WARN_ON(!group->owner_cnt || !group->owner))
>  		goto unlock_out;
>  
>  	group->owner_cnt = 0;
> -	/*
> -	 * The UNMANAGED domain should be detached before all USER
> -	 * owners have been released.
> -	 */
> -	if (!WARN_ON(group->domain) && group->default_domain)
> -		__iommu_attach_group(group->default_domain, group);
>  	group->owner = NULL;
> +	ret = __iommu_group_set_domain(group, group->default_domain);
> +	WARN(ret, "iommu driver failed to attach the default domain");
> +
>  unlock_out:
>  	mutex_unlock(&group->mutex);
>  }
> 
> base-commit: da844db4722bdd333142b40f0e414e2aedc2a4c0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Jason Gunthorpe via iommu]

On Wed, May 18, 2022 at 02:50:36PM -0400, Eric Farman wrote:

> I got a heads up from Matt about the s390 KVM vfio- variants failing on
> linux-next.
> 
> For vfio-ap and vfio-ccw, they fail on the above error. Both calls to
> __iommu_domain_alloc fail because while dev->dev->bus is non-NULL (it
> points to the mdev bus_type registered in mdev_init()), the bus-
> >iommu_ops pointer is NULL. Which makes sense; the iommu_group is vfio-
> noiommu, via vfio_register_emulated_iommu_dev(), and mdev didn't
> establish an iommu_ops for its bus.

Oh, I think this is a VFIO problem, the iommu layer should not have to
deal with these fake non-iommu groups.

From 9884850a5ceac957e6715beab0888294d4088877 Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgg@nvidia.com>
Date: Wed, 18 May 2022 16:03:34 -0300
Subject: [PATCH] vfio: Do not manipulate iommu dma_owner for fake iommu groups

Since asserting dma ownership now causes the group to have its DMA blocked
the iommu layer requires a working iommu. This means the dma_owner APIs
cannot be used on the fake groups that VFIO creates. Test for this and
avoid calling them.

Otherwise asserting dma ownership will fail for VFIO mdev devices as a
BLOCKING iommu_domain cannot be allocated due to the NULL iommu ops.

Fixes: 0286300e6045 ("iommu: iommu_group_claim_dma_owner() must always assign a domain")
Reported-by: Eric Farman <farman@linux.ibm.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

I think this will have to go through Alex's tree due to all the other rework
in this area.

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index cfcff7764403fc..f5ed03897210c3 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -927,7 +927,8 @@ static void __vfio_group_unset_container(struct vfio_group *group)
 		driver->ops->detach_group(container->iommu_data,
 					  group->iommu_group);
 
-	iommu_group_release_dma_owner(group->iommu_group);
+	if (group->type == VFIO_IOMMU)
+		iommu_group_release_dma_owner(group->iommu_group);
 
 	group->container = NULL;
 	group->container_users = 0;
@@ -1001,9 +1002,11 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
 		goto unlock_out;
 	}
 
-	ret = iommu_group_claim_dma_owner(group->iommu_group, f.file);
-	if (ret)
-		goto unlock_out;
+	if (group->type == VFIO_IOMMU) {
+		ret = iommu_group_claim_dma_owner(group->iommu_group, f.file);
+		if (ret)
+			goto unlock_out;
+	}
 
 	driver = container->iommu_driver;
 	if (driver) {
@@ -1011,7 +1014,9 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
 						group->iommu_group,
 						group->type);
 		if (ret) {
-			iommu_group_release_dma_owner(group->iommu_group);
+			if (group->type == VFIO_IOMMU)
+				iommu_group_release_dma_owner(
+					group->iommu_group);
 			goto unlock_out;
 		}
 	}
-- 
2.36.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Eric Farman]

On Wed, 2022-05-18 at 16:14 -0300, Jason Gunthorpe wrote:
> On Wed, May 18, 2022 at 02:50:36PM -0400, Eric Farman wrote:
> 
> > I got a heads up from Matt about the s390 KVM vfio- variants
> > failing on
> > linux-next.
> > 
> > For vfio-ap and vfio-ccw, they fail on the above error. Both calls
> > to
> > __iommu_domain_alloc fail because while dev->dev->bus is non-NULL
> > (it
> > points to the mdev bus_type registered in mdev_init()), the bus-
> > > iommu_ops pointer is NULL. Which makes sense; the iommu_group is
> > > vfio-
> > noiommu, via vfio_register_emulated_iommu_dev(), and mdev didn't
> > establish an iommu_ops for its bus.
> 
> Oh, I think this is a VFIO problem, the iommu layer should not have
> to
> deal with these fake non-iommu groups.
> 
> From 9884850a5ceac957e6715beab0888294d4088877 Mon Sep 17 00:00:00
> 2001
> From: Jason Gunthorpe <jgg@nvidia.com>
> Date: Wed, 18 May 2022 16:03:34 -0300
> Subject: [PATCH] vfio: Do not manipulate iommu dma_owner for fake
> iommu groups
> 
> Since asserting dma ownership now causes the group to have its DMA
> blocked
> the iommu layer requires a working iommu. This means the dma_owner
> APIs
> cannot be used on the fake groups that VFIO creates. Test for this
> and
> avoid calling them.
> 
> Otherwise asserting dma ownership will fail for VFIO mdev devices as
> a
> BLOCKING iommu_domain cannot be allocated due to the NULL iommu ops.
> 
> Fixes: 0286300e6045 ("iommu: iommu_group_claim_dma_owner() must
> always assign a domain")
> Reported-by: Eric Farman <farman@linux.ibm.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Ah, nice. That takes care of it for me, thank you!

Tested-by: Eric Farman <farman@linux.ibm.com>

> ---
>  drivers/vfio/vfio.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> I think this will have to go through Alex's tree due to all the other
> rework
> in this area.
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index cfcff7764403fc..f5ed03897210c3 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -927,7 +927,8 @@ static void __vfio_group_unset_container(struct
> vfio_group *group)
>  		driver->ops->detach_group(container->iommu_data,
>  					  group->iommu_group);
>  
> -	iommu_group_release_dma_owner(group->iommu_group);
> +	if (group->type == VFIO_IOMMU)
> +		iommu_group_release_dma_owner(group->iommu_group);
>  
>  	group->container = NULL;
>  	group->container_users = 0;
> @@ -1001,9 +1002,11 @@ static int vfio_group_set_container(struct
> vfio_group *group, int container_fd)
>  		goto unlock_out;
>  	}
>  
> -	ret = iommu_group_claim_dma_owner(group->iommu_group, f.file);
> -	if (ret)
> -		goto unlock_out;
> +	if (group->type == VFIO_IOMMU) {
> +		ret = iommu_group_claim_dma_owner(group->iommu_group,
> f.file);
> +		if (ret)
> +			goto unlock_out;
> +	}
>  
>  	driver = container->iommu_driver;
>  	if (driver) {
> @@ -1011,7 +1014,9 @@ static int vfio_group_set_container(struct
> vfio_group *group, int container_fd)
>  						group->iommu_group,
>  						group->type);
>  		if (ret) {
> -			iommu_group_release_dma_owner(group-
> >iommu_group);
> +			if (group->type == VFIO_IOMMU)
> +				iommu_group_release_dma_owner(
> +					group->iommu_group);
>  			goto unlock_out;
>  		}
>  	}

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Joerg Roedel]

On Wed, May 18, 2022 at 04:14:46PM -0300, Jason Gunthorpe wrote:
> Fixes: 0286300e6045 ("iommu: iommu_group_claim_dma_owner() must always assign a domain")
> Reported-by: Eric Farman <farman@linux.ibm.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/vfio.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

That fix will be taken care of by Alex? Or does it need to go through
the IOMMU tree?

Regards,

-- 
Jörg Rödel
jroedel@suse.de

SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
 
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Alex Williamson]

On Thu, 19 May 2022 09:32:05 +0200
Joerg Roedel <jroedel@suse.de> wrote:

> On Wed, May 18, 2022 at 04:14:46PM -0300, Jason Gunthorpe wrote:
> > Fixes: 0286300e6045 ("iommu: iommu_group_claim_dma_owner() must always assign a domain")
> > Reported-by: Eric Farman <farman@linux.ibm.com>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  drivers/vfio/vfio.c | 15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)  
> 
> That fix will be taken care of by Alex? Or does it need to go through
> the IOMMU tree?

The comment suggested my tree.  Jason, were you planning to post this
on its own instead of buried in a reply or shall we take it from here?
Thanks,

Alex

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Jason Gunthorpe via iommu]

On Thu, May 19, 2022 at 10:51:47AM -0600, Alex Williamson wrote:
> On Thu, 19 May 2022 09:32:05 +0200
> Joerg Roedel <jroedel@suse.de> wrote:
> 
> > On Wed, May 18, 2022 at 04:14:46PM -0300, Jason Gunthorpe wrote:
> > > Fixes: 0286300e6045 ("iommu: iommu_group_claim_dma_owner() must always assign a domain")
> > > Reported-by: Eric Farman <farman@linux.ibm.com>
> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > >  drivers/vfio/vfio.c | 15 ++++++++++-----
> > >  1 file changed, 10 insertions(+), 5 deletions(-)  
> > 
> > That fix will be taken care of by Alex? Or does it need to go through
> > the IOMMU tree?
> 
> The comment suggested my tree.  Jason, were you planning to post this
> on its own instead of buried in a reply or shall we take it from
> here?

Let me repost it with a proper cc list, I think your tree is best.

Thanks,
Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu