[PATCH v2 00/14] Consolidate the error handling around device attachment

[PATCH v2 00/14] Consolidate the error handling around device attachment

[Jason Gunthorpe]

Device attachment has a bunch of different flows open coded in different
ways throughout the code.

One of the things that became apparently recently is that error handling
is important and we do need to consistently treat errors during attach and
have some strategy to unwind back to a safe state.

Implement a single algorithm for this in one function. It will call each
device's attach, if it fails it will try to go back to the prior domain or
as a contingency against a UAF crash try to go to a blocking domain.

As part of this we consolidate how the default domain is created and
attached as well into one place with a consistent flow.

The new worker functions are called __iommu_device_set_domain() and
__iommu_group_set_domain_internal(), each has sensible error handling
internally. At the end __iommu_group_set_domain_internal() is the only
function that stores to group->domain, and must be called to change this
value.

Some flags tell the intent of the caller, if the caller cannot accept a
failure, or if the caller is a first attach and wants to do the deferred
logic.

Several of the confusing workflows where we store things in group->domain
or group->default_domain before they are fully setup are removed.

This has a followup series that does a similar de-duplication to the probe
path:

https://github.com/jgunthorpe/linux/commits/iommu_err_unwind

v2:
 - New patch to remove iommu_group_device_count()
 - New patch to add a for_each helper: for_each_group_device()
 - Rebase on Joerg's tree
 - IOMMU_SET_DOMAIN_MUST_SUCCEED instead of true
 - Split patch to fix owned groups during first attach
 - Change iommu_create_device_direct_mappings to accept a domain not a
   group
 - Significantly revise the "iommu: Consolidate the default_domain setup to
   one function" patch to de-duplicate the domain type calculation logic
   too
 - New patch to clean the flow inside iommu_group_store_type()
v1: https://lore.kernel.org/r/0-v1-20507a7e6b7e+2d6-iommu_err_unwind_jgg@nvidia.com

Jason Gunthorpe (14):
  iommu: Replace iommu_group_device_count() with list_count_nodes()
  iommu: Add for_each_group_device()
  iommu: Make __iommu_group_set_domain() handle error unwind
  iommu: Use __iommu_group_set_domain() for __iommu_attach_group()
  iommu: Use __iommu_group_set_domain() in iommu_change_dev_def_domain()
  iommu: Replace __iommu_group_dma_first_attach() with set_domain
  iommu: Make iommu_group_do_dma_first_attach() simpler
  iommu: Make iommu_group_do_dma_first_attach() work with owned groups
  iommu: Fix iommu_probe_device() to attach the right domain
  iommu: Remove the assignment of group->domain during default domain
    alloc
  iommu: Consolidate the code to calculate the target default domain
    type
  iommu: Consolidate the default_domain setup to one function
  iommu: Remove __iommu_group_for_each_dev()
  iommu: Tidy the control flow in iommu_group_store_type()

 .clang-format         |   1 +
 drivers/iommu/iommu.c | 623 ++++++++++++++++++++----------------------
 2 files changed, 293 insertions(+), 331 deletions(-)


base-commit: 3578e36f238ef81a1967e849e0a3106c9dd37e68
-- 
2.40.0

[PATCH v2 01/14] iommu: Replace iommu_group_device_count() with list_count_nodes()

[Jason Gunthorpe]

No reason to wrapper a standard function, just call the library directly.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7abee83610b6c9..8cdf47279704c2 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1149,17 +1149,6 @@ void iommu_group_remove_device(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_group_remove_device);
 
-static int iommu_group_device_count(struct iommu_group *group)
-{
-	struct group_device *entry;
-	int ret = 0;
-
-	list_for_each_entry(entry, &group->devices, list)
-		ret++;
-
-	return ret;
-}
-
 static int __iommu_group_for_each_dev(struct iommu_group *group, void *data,
 				      int (*fn)(struct device *, void *))
 {
@@ -2101,7 +2090,7 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 	 */
 	mutex_lock(&group->mutex);
 	ret = -EINVAL;
-	if (iommu_group_device_count(group) != 1)
+	if (list_count_nodes(&group->devices) != 1)
 		goto out_unlock;
 
 	ret = __iommu_attach_group(domain, group);
@@ -2132,7 +2121,7 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
 
 	mutex_lock(&group->mutex);
 	if (WARN_ON(domain != group->domain) ||
-	    WARN_ON(iommu_group_device_count(group) != 1))
+	    WARN_ON(list_count_nodes(&group->devices) != 1))
 		goto out_unlock;
 	__iommu_group_set_core_domain(group);
 
-- 
2.40.0

Re: [PATCH v2 01/14] iommu: Replace iommu_group_device_count() with list_count_nodes()

[Baolu Lu]

On 3/30/23 7:40 AM, Jason Gunthorpe wrote:
> No reason to wrapper a standard function, just call the library directly.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

RE: [PATCH v2 01/14] iommu: Replace iommu_group_device_count() with list_count_nodes()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, March 30, 2023 7:40 AM
> 
> No reason to wrapper a standard function, just call the library directly.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

[PATCH v2 02/14] iommu: Add for_each_group_device()

[Jason Gunthorpe]

Convenience macro to iterate over every struct group_device in the group.

Replace all open coded list_for_each_entry's with this macro.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 .clang-format         |  1 +
 drivers/iommu/iommu.c | 16 ++++++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/.clang-format b/.clang-format
index d988e9fa9b2653..bece1995f2c159 100644
--- a/.clang-format
+++ b/.clang-format
@@ -254,6 +254,7 @@ ForEachMacros:
   - 'for_each_free_mem_range'
   - 'for_each_free_mem_range_reverse'
   - 'for_each_func_rsrc'
+  - 'for_each_group_device'
   - 'for_each_group_evsel'
   - 'for_each_group_member'
   - 'for_each_hstate'
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8cdf47279704c2..501257bd02fc3c 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -67,6 +67,10 @@ struct group_device {
 	char *name;
 };
 
+/* Iterate over each struct group_device in a struct iommu_group */
+#define for_each_group_device(group, pos) \
+	list_for_each_entry(pos, &(group)->devices, list)
+
 struct iommu_group_attribute {
 	struct attribute attr;
 	ssize_t (*show)(struct iommu_group *group, char *buf);
@@ -463,7 +467,7 @@ __iommu_group_remove_device(struct iommu_group *group, struct device *dev)
 	struct group_device *device;
 
 	lockdep_assert_held(&group->mutex);
-	list_for_each_entry(device, &group->devices, list) {
+	for_each_group_device(group, device) {
 		if (device->dev == dev) {
 			list_del(&device->list);
 			return device;
@@ -702,7 +706,7 @@ int iommu_get_group_resv_regions(struct iommu_group *group,
 	int ret = 0;
 
 	mutex_lock(&group->mutex);
-	list_for_each_entry(device, &group->devices, list) {
+	for_each_group_device(group, device) {
 		struct list_head dev_resv_regions;
 
 		/*
@@ -1155,7 +1159,7 @@ static int __iommu_group_for_each_dev(struct iommu_group *group, void *data,
 	struct group_device *device;
 	int ret = 0;
 
-	list_for_each_entry(device, &group->devices, list) {
+	for_each_group_device(group, device) {
 		ret = fn(device->dev, data);
 		if (ret)
 			break;
@@ -1959,7 +1963,7 @@ bool iommu_group_has_isolated_msi(struct iommu_group *group)
 	bool ret = true;
 
 	mutex_lock(&group->mutex);
-	list_for_each_entry(group_dev, &group->devices, list)
+	for_each_group_device(group, group_dev)
 		ret &= msi_device_has_isolated_msi(group_dev->dev);
 	mutex_unlock(&group->mutex);
 	return ret;
@@ -3261,7 +3265,7 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
 	struct group_device *device;
 	int ret = 0;
 
-	list_for_each_entry(device, &group->devices, list) {
+	for_each_group_device(group, device) {
 		ret = domain->ops->set_dev_pasid(domain, device->dev, pasid);
 		if (ret)
 			break;
@@ -3276,7 +3280,7 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
 	struct group_device *device;
 	const struct iommu_ops *ops;
 
-	list_for_each_entry(device, &group->devices, list) {
+	for_each_group_device(group, device) {
 		ops = dev_iommu_ops(device->dev);
 		ops->remove_dev_pasid(device->dev, pasid);
 	}
-- 
2.40.0

Re: [PATCH v2 02/14] iommu: Add for_each_group_device()

[Miguel Ojeda]

On Thu, Mar 30, 2023 at 1:40 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> diff --git a/.clang-format b/.clang-format
> index d988e9fa9b2653..bece1995f2c159 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -254,6 +254,7 @@ ForEachMacros:
>    - 'for_each_free_mem_range'
>    - 'for_each_free_mem_range_reverse'
>    - 'for_each_func_rsrc'
> +  - 'for_each_group_device'

We currently only go through `include/` and `tools/` to generate that
list, so this could easily get deleted on an update. But we discussed
going through more files, or we could have some extra explicit items,
etc.

Since you added it manually (i.e. vs. renaming some entries), does it
mean you are already using `clang-format` for this?

Thanks!

Cheers,
Miguel

Re: [PATCH v2 02/14] iommu: Add for_each_group_device()

[Jason Gunthorpe]

On Thu, Mar 30, 2023 at 01:52:15AM +0200, Miguel Ojeda wrote:
> On Thu, Mar 30, 2023 at 1:40 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > diff --git a/.clang-format b/.clang-format
> > index d988e9fa9b2653..bece1995f2c159 100644
> > --- a/.clang-format
> > +++ b/.clang-format
> > @@ -254,6 +254,7 @@ ForEachMacros:
> >    - 'for_each_free_mem_range'
> >    - 'for_each_free_mem_range_reverse'
> >    - 'for_each_func_rsrc'
> > +  - 'for_each_group_device'
> 
> We currently only go through `include/` and `tools/` to generate that
> list, so this could easily get deleted on an update. But we discussed
> going through more files, or we could have some extra explicit items,
> etc.

That would be nice..

> Since you added it manually (i.e. vs. renaming some entries), does it
> mean you are already using `clang-format` for this?

Yes!

clang-format and clangd have really transformed how I'm working!

Jason

Re: [PATCH v2 02/14] iommu: Add for_each_group_device()

[Miguel Ojeda]

On Thu, Mar 30, 2023 at 4:28 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> That would be nice..

Done! https://lore.kernel.org/lkml/20230509125812.310307-2-ojeda@kernel.org/

> Yes!
>
> clang-format and clangd have really transformed how I'm working!

Thanks, I am glad that is the case, and it was also useful to justify
the change above.

Cheers,
Miguel

Re: [PATCH v2 02/14] iommu: Add for_each_group_device()

[Jason Gunthorpe]

On Tue, May 09, 2023 at 03:12:08PM +0200, Miguel Ojeda wrote:

> > Yes!
> >
> > clang-format and clangd have really transformed how I'm working!
> 
> Thanks, I am glad that is the case, and it was also useful to justify
> the change above.

Thanks!

Hey.. with all your clang folk attention - any chance we can get
clangd fixed for kernel usage to not ignore __* functions? It is
really annoying for our usual kernel patterns

Jason

Re: [PATCH v2 02/14] iommu: Add for_each_group_device()

[Baolu Lu]

On 3/30/23 7:40 AM, Jason Gunthorpe wrote:
> Convenience macro to iterate over every struct group_device in the group.
> 
> Replace all open coded list_for_each_entry's with this macro.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

RE: [PATCH v2 02/14] iommu: Add for_each_group_device()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, March 30, 2023 7:40 AM
> 
> Convenience macro to iterate over every struct group_device in the group.
> 
> Replace all open coded list_for_each_entry's with this macro.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

[PATCH v2 03/14] iommu: Make __iommu_group_set_domain() handle error unwind

[Jason Gunthorpe]

Let's try to have a consistent and clear strategy for error handling
during domain attach failures.

There are two broad categories, the first is callers doing destruction and
trying to set the domain back to a previously good domain. These cases
cannot handle failure during destruction flows and must succeed, or at
least avoid a UAF on the current group->domain which is likely about to be
freed.

Many of the drivers are well behaved here and will not hit the WARN_ON's
or a UAF, but some are doing hypercalls/etc that can fail unpredictably
and don't meet the expectations.

The second case is attaching a domain for the first time in a failable
context, failure should restore the attachment back to group->domain using
the above unfailable operation.

Have __iommu_group_set_domain_internal() execute a common algorithm that
tries to achieve this, and in the worst case, would leave a device
"detached" or assigned to a global blocking domain. This relies on some
existing common driver behaviors where attach failure will also do detatch
and true IOMMU_DOMAIN_BLOCK implementations that are not allowed to ever
fail.

Name the first case with __iommu_group_set_domain_nofail() to make it
clear.

Pull all the error handling and WARN_ON generation into
__iommu_group_set_domain_internal().

Avoid the obfuscating use of __iommu_group_for_each_dev() and be more
careful about what should happen during failures by only touching devices
we've already touched.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 130 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 108 insertions(+), 22 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 501257bd02fc3c..b7cb2a56ebfe8c 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -99,8 +99,26 @@ 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);
+
+enum {
+	IOMMU_SET_DOMAIN_MUST_SUCCEED = 1 << 0,
+};
+
+static int __iommu_group_set_domain_internal(struct iommu_group *group,
+					     struct iommu_domain *new_domain,
+					     unsigned int flags);
 static int __iommu_group_set_domain(struct iommu_group *group,
-				    struct iommu_domain *new_domain);
+				    struct iommu_domain *new_domain)
+{
+	return __iommu_group_set_domain_internal(group, new_domain, 0);
+}
+static void __iommu_group_set_domain_nofail(struct iommu_group *group,
+					    struct iommu_domain *new_domain)
+{
+	WARN_ON(__iommu_group_set_domain_internal(
+		group, new_domain, IOMMU_SET_DOMAIN_MUST_SUCCEED));
+}
+
 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);
@@ -2040,15 +2058,13 @@ EXPORT_SYMBOL_GPL(iommu_domain_free);
 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");
+	__iommu_group_set_domain_nofail(group, new_domain);
 }
 
 static int __iommu_attach_device(struct iommu_domain *domain,
@@ -2233,21 +2249,55 @@ int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
 }
 EXPORT_SYMBOL_GPL(iommu_attach_group);
 
-static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
+static int __iommu_device_set_domain(struct iommu_group *group,
+				     struct device *dev,
+				     struct iommu_domain *new_domain,
+				     unsigned int flags)
 {
-	const struct iommu_ops *ops = dev_iommu_ops(dev);
-
-	if (!WARN_ON(!ops->set_platform_dma_ops))
-		ops->set_platform_dma_ops(dev);
+	int ret;
 
+	ret = __iommu_attach_device(new_domain, dev);
+	if (ret) {
+		/*
+		 * If we have a blocking domain then try to attach that in hopes
+		 * of avoiding a UAF. Modern drivers should implement blocking
+		 * domains as global statics that cannot fail.
+		 */
+		if ((flags & IOMMU_SET_DOMAIN_MUST_SUCCEED) &&
+		    group->blocking_domain &&
+		    group->blocking_domain != new_domain)
+			__iommu_attach_device(group->blocking_domain, dev);
+		return ret;
+	}
 	return 0;
 }
 
-static int __iommu_group_set_domain(struct iommu_group *group,
-				    struct iommu_domain *new_domain)
+/*
+ * If 0 is returned the group's domain is new_domain. If an error is returned
+ * then the group's domain will be set back to the existing domain unless
+ * IOMMU_SET_DOMAIN_MUST_SUCCEED, otherwise an error is returned and the group's
+ * domains is left inconsistent. This is a driver bug to fail attach with a
+ * previously good domain. We try to avoid a kernel UAF because of this.
+ *
+ * IOMMU groups are really the natural working unit of the IOMMU, but the IOMMU
+ * API works on domains and devices.  Bridge that gap by iterating over the
+ * devices in a group.  Ideally we'd have a single device which represents the
+ * requestor ID of the group, but we also allow IOMMU drivers to create policy
+ * defined minimum sets, where the physical hardware may be able to distiguish
+ * members, but we wish to group them at a higher level (ex. untrusted
+ * multi-function PCI devices).  Thus we attach each device.
+ */
+static int __iommu_group_set_domain_internal(struct iommu_group *group,
+					     struct iommu_domain *new_domain,
+					     unsigned int flags)
 {
+	struct group_device *last_gdev;
+	struct group_device *gdev;
+	int result;
 	int ret;
 
+	lockdep_assert_held(&group->mutex);
+
 	if (group->domain == new_domain)
 		return 0;
 
@@ -2257,8 +2307,12 @@ static int __iommu_group_set_domain(struct iommu_group *group,
 	 * platform specific behavior.
 	 */
 	if (!new_domain) {
-		__iommu_group_for_each_dev(group, NULL,
-					   iommu_group_do_set_platform_dma);
+		for_each_group_device(group, gdev) {
+			const struct iommu_ops *ops = dev_iommu_ops(gdev->dev);
+
+			if (!WARN_ON(!ops->set_platform_dma_ops))
+				ops->set_platform_dma_ops(gdev->dev);
+		}
 		group->domain = NULL;
 		return 0;
 	}
@@ -2272,12 +2326,47 @@ static int __iommu_group_set_domain(struct iommu_group *group,
 	 * 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)
-		return ret;
+	result = 0;
+	for_each_group_device(group, gdev) {
+		ret = __iommu_device_set_domain(group, gdev->dev, new_domain,
+						flags);
+		if (ret) {
+			result = ret;
+			/*
+			 * Keep trying the other devices in the group. If a
+			 * driver fails attach to an otherwise good domain, and
+			 * does not support blocking domains, it should at least
+			 * drop its reference on the current domain so we don't
+			 * UAF.
+			 */
+			if (flags & IOMMU_SET_DOMAIN_MUST_SUCCEED)
+				continue;
+			goto err_revert;
+		}
+	}
 	group->domain = new_domain;
-	return 0;
+	return result;
+
+err_revert:
+	last_gdev = gdev;
+	for_each_group_device(group, gdev) {
+		const struct iommu_ops *ops = dev_iommu_ops(gdev->dev);
+
+		/*
+		 * If set_platform_dma_ops is not present a NULL domain can
+		 * happen only for first probe, in which case we leave
+		 * group->domain as NULL and let release clean everything up.
+		 */
+		if (group->domain)
+			WARN_ON(__iommu_device_set_domain(
+				group, gdev->dev, group->domain,
+				IOMMU_SET_DOMAIN_MUST_SUCCEED));
+		else if (ops->set_platform_dma_ops)
+			ops->set_platform_dma_ops(gdev->dev);
+		if (gdev == last_gdev)
+			break;
+	}
+	return ret;
 }
 
 void iommu_detach_group(struct iommu_domain *domain, struct iommu_group *group)
@@ -3194,16 +3283,13 @@ EXPORT_SYMBOL_GPL(iommu_device_claim_dma_owner);
 
 static void __iommu_release_dma_ownership(struct iommu_group *group)
 {
-	int ret;
-
 	if (WARN_ON(!group->owner_cnt || !group->owner ||
 		    !xa_empty(&group->pasid_array)))
 		return;
 
 	group->owner_cnt = 0;
 	group->owner = NULL;
-	ret = __iommu_group_set_domain(group, group->default_domain);
-	WARN(ret, "iommu driver failed to attach the default domain");
+	__iommu_group_set_domain_nofail(group, group->default_domain);
 }
 
 /**
-- 
2.40.0

Re: [PATCH v2 03/14] iommu: Make __iommu_group_set_domain() handle error unwind

[Baolu Lu]

On 3/30/23 7:40 AM, Jason Gunthorpe wrote:
> Let's try to have a consistent and clear strategy for error handling
> during domain attach failures.
> 
> There are two broad categories, the first is callers doing destruction and
> trying to set the domain back to a previously good domain. These cases
> cannot handle failure during destruction flows and must succeed, or at
> least avoid a UAF on the current group->domain which is likely about to be
> freed.
> 
> Many of the drivers are well behaved here and will not hit the WARN_ON's
> or a UAF, but some are doing hypercalls/etc that can fail unpredictably
> and don't meet the expectations.
> 
> The second case is attaching a domain for the first time in a failable
> context, failure should restore the attachment back to group->domain using
> the above unfailable operation.
> 
> Have __iommu_group_set_domain_internal() execute a common algorithm that
> tries to achieve this, and in the worst case, would leave a device
> "detached" or assigned to a global blocking domain. This relies on some
> existing common driver behaviors where attach failure will also do detatch
> and true IOMMU_DOMAIN_BLOCK implementations that are not allowed to ever
> fail.
> 
> Name the first case with __iommu_group_set_domain_nofail() to make it
> clear.
> 
> Pull all the error handling and WARN_ON generation into
> __iommu_group_set_domain_internal().
> 
> Avoid the obfuscating use of __iommu_group_for_each_dev() and be more
> careful about what should happen during failures by only touching devices
> we've already touched.
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

[PATCH v2 04/14] iommu: Use __iommu_group_set_domain() for __iommu_attach_group()

[Jason Gunthorpe]

The error recovery here matches the recovery inside
__iommu_group_set_domain(), so just use it directly.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 40 +---------------------------------------
 1 file changed, 1 insertion(+), 39 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b7cb2a56ebfe8c..ba9e988293f23f 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2177,52 +2177,14 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
 	return dev->iommu_group->default_domain;
 }
 
-/*
- * IOMMU groups are really the natural working unit of the IOMMU, but
- * the IOMMU API works on domains and devices.  Bridge that gap by
- * iterating over the devices in a group.  Ideally we'd have a single
- * device which represents the requestor ID of the group, but we also
- * allow IOMMU drivers to create policy defined minimum sets, where
- * the physical hardware may be able to distiguish members, but we
- * wish to group them at a higher level (ex. untrusted multi-function
- * PCI devices).  Thus we attach each device.
- */
-static int iommu_group_do_attach_device(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-
-	return __iommu_attach_device(domain, dev);
-}
-
 static int __iommu_attach_group(struct iommu_domain *domain,
 				struct iommu_group *group)
 {
-	int ret;
-
 	if (group->domain && group->domain != group->default_domain &&
 	    group->domain != group->blocking_domain)
 		return -EBUSY;
 
-	ret = __iommu_group_for_each_dev(group, domain,
-					 iommu_group_do_attach_device);
-	if (ret == 0) {
-		group->domain = domain;
-	} else {
-		/*
-		 * To recover from the case when certain device within the
-		 * group fails to attach to the new domain, we need force
-		 * attaching all devices back to the old domain. The old
-		 * domain is compatible for all devices in the group,
-		 * hence the iommu driver should always return success.
-		 */
-		struct iommu_domain *old_domain = group->domain;
-
-		group->domain = NULL;
-		WARN(__iommu_group_set_domain(group, old_domain),
-		     "iommu driver failed to attach a compatible domain");
-	}
-
-	return ret;
+	return __iommu_group_set_domain(group, domain);
 }
 
 /**
-- 
2.40.0

Re: [PATCH v2 04/14] iommu: Use __iommu_group_set_domain() for __iommu_attach_group()

[Baolu Lu]

On 3/30/23 7:40 AM, Jason Gunthorpe wrote:
> The error recovery here matches the recovery inside
> __iommu_group_set_domain(), so just use it directly.
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

[PATCH v2 05/14] iommu: Use __iommu_group_set_domain() in iommu_change_dev_def_domain()

[Jason Gunthorpe]

This is missing re-attach error handling if the attach fails, use the
common code.

The ugly "group->domain = prev_domain" will be cleaned in a later patch.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ba9e988293f23f..cd75d7405a3051 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2963,11 +2963,12 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 	if (ret)
 		goto restore_old_domain;
 
-	ret = iommu_group_create_direct_mappings(group);
+	group->domain = prev_dom;
+	ret = iommu_create_device_direct_mappings(group, dev);
 	if (ret)
 		goto free_new_domain;
 
-	ret = __iommu_attach_group(group->default_domain, group);
+	ret = __iommu_group_set_domain(group, group->default_domain);
 	if (ret)
 		goto free_new_domain;
 
@@ -2979,7 +2980,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 	iommu_domain_free(group->default_domain);
 restore_old_domain:
 	group->default_domain = prev_dom;
-	group->domain = prev_dom;
 
 	return ret;
 }
-- 
2.40.0

Re: [PATCH v2 05/14] iommu: Use __iommu_group_set_domain() in iommu_change_dev_def_domain()

[Baolu Lu]

On 3/30/23 7:40 AM, Jason Gunthorpe wrote:
> This is missing re-attach error handling if the attach fails, use the
> common code.
> 
> The ugly "group->domain = prev_domain" will be cleaned in a later patch.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

RE: [PATCH v2 05/14] iommu: Use __iommu_group_set_domain() in iommu_change_dev_def_domain()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, March 30, 2023 7:40 AM
> 
> This is missing re-attach error handling if the attach fails, use the
> common code.
> 
> The ugly "group->domain = prev_domain" will be cleaned in a later patch.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

[PATCH v2 06/14] iommu: Replace __iommu_group_dma_first_attach() with set_domain

[Jason Gunthorpe]

Prepare for removing the group->domain set from
iommu_group_alloc_default_domain() by calling
__iommu_group_set_domain_internal() to set the group->domain.

Add IOMMU_SET_DOMAIN_WITH_DEFERRED to allow it to do the attach_deferred
logic.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index cd75d7405a3051..af3af752c255e4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -102,6 +102,7 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 
 enum {
 	IOMMU_SET_DOMAIN_MUST_SUCCEED = 1 << 0,
+	IOMMU_SET_DOMAIN_WITH_DEFERRED = 1 << 1,
 };
 
 static int __iommu_group_set_domain_internal(struct iommu_group *group,
@@ -1855,12 +1856,6 @@ static void probe_alloc_default_domain(struct bus_type *bus,
 
 }
 
-static int __iommu_group_dma_first_attach(struct iommu_group *group)
-{
-	return __iommu_group_for_each_dev(group, group->default_domain,
-					  iommu_group_do_dma_first_attach);
-}
-
 static int iommu_group_do_probe_finalize(struct device *dev, void *data)
 {
 	const struct iommu_ops *ops = dev_iommu_ops(dev);
@@ -1923,7 +1918,10 @@ int bus_iommu_probe(struct bus_type *bus)
 
 		iommu_group_create_direct_mappings(group);
 
-		ret = __iommu_group_dma_first_attach(group);
+		group->domain = NULL;
+		ret = __iommu_group_set_domain_internal(
+			group, group->default_domain,
+			IOMMU_SET_DOMAIN_WITH_DEFERRED);
 
 		mutex_unlock(&group->mutex);
 
@@ -2218,6 +2216,12 @@ static int __iommu_device_set_domain(struct iommu_group *group,
 {
 	int ret;
 
+	if ((flags & IOMMU_SET_DOMAIN_WITH_DEFERRED) &&
+	    iommu_is_attach_deferred(dev)) {
+		dev->iommu->attach_deferred = 1;
+		return 0;
+	}
+
 	ret = __iommu_attach_device(new_domain, dev);
 	if (ret) {
 		/*
-- 
2.40.0

Re: [PATCH v2 06/14] iommu: Replace __iommu_group_dma_first_attach() with set_domain

[Baolu Lu]

On 3/30/23 7:40 AM, Jason Gunthorpe wrote:
> Prepare for removing the group->domain set from
> iommu_group_alloc_default_domain() by calling
> __iommu_group_set_domain_internal() to set the group->domain.
> 
> Add IOMMU_SET_DOMAIN_WITH_DEFERRED to allow it to do the attach_deferred
> logic.
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

[PATCH v2 07/14] iommu: Make iommu_group_do_dma_first_attach() simpler

[Jason Gunthorpe]

It should always attach to the current group->domain, so don't take in a
domain parameter. Use the __iommu_device_set_domain() common code to
handle the attach.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index af3af752c255e4..c68cf551d05dc4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -105,6 +105,10 @@ enum {
 	IOMMU_SET_DOMAIN_WITH_DEFERRED = 1 << 1,
 };
 
+static int __iommu_device_set_domain(struct iommu_group *group,
+				     struct device *dev,
+				     struct iommu_domain *new_domain,
+				     unsigned int flags);
 static int __iommu_group_set_domain_internal(struct iommu_group *group,
 					     struct iommu_domain *new_domain,
 					     unsigned int flags);
@@ -405,18 +409,10 @@ static bool iommu_is_attach_deferred(struct device *dev)
 	return false;
 }
 
-static int iommu_group_do_dma_first_attach(struct device *dev, void *data)
+static int iommu_group_do_dma_first_attach(struct iommu_group *group, struct device *dev)
 {
-	struct iommu_domain *domain = data;
-
-	lockdep_assert_held(&dev->iommu_group->mutex);
-
-	if (iommu_is_attach_deferred(dev)) {
-		dev->iommu->attach_deferred = 1;
-		return 0;
-	}
-
-	return __iommu_attach_device(domain, dev);
+	return __iommu_device_set_domain(group, dev, group->domain,
+					 IOMMU_SET_DOMAIN_WITH_DEFERRED);
 }
 
 int iommu_probe_device(struct device *dev)
@@ -449,7 +445,7 @@ int iommu_probe_device(struct device *dev)
 	 * attach the default domain.
 	 */
 	if (group->default_domain && !group->owner) {
-		ret = iommu_group_do_dma_first_attach(dev, group->default_domain);
+		ret = iommu_group_do_dma_first_attach(group, dev);
 		if (ret) {
 			mutex_unlock(&group->mutex);
 			iommu_group_put(group);
@@ -1117,7 +1113,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 	mutex_lock(&group->mutex);
 	list_add_tail(&device->list, &group->devices);
 	if (group->domain)
-		ret = iommu_group_do_dma_first_attach(dev, group->domain);
+		ret = iommu_group_do_dma_first_attach(group, dev);
 	mutex_unlock(&group->mutex);
 	if (ret)
 		goto err_put_group;
-- 
2.40.0

Re: [PATCH v2 07/14] iommu: Make iommu_group_do_dma_first_attach() simpler

[Baolu Lu]

On 3/30/23 7:40 AM, Jason Gunthorpe wrote:
> It should always attach to the current group->domain, so don't take in a
> domain parameter. Use the __iommu_device_set_domain() common code to
> handle the attach.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/iommu.c | 22 +++++++++-------------
>   1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index af3af752c255e4..c68cf551d05dc4 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -105,6 +105,10 @@ enum {
>   	IOMMU_SET_DOMAIN_WITH_DEFERRED = 1 << 1,
>   };
>   
> +static int __iommu_device_set_domain(struct iommu_group *group,
> +				     struct device *dev,
> +				     struct iommu_domain *new_domain,
> +				     unsigned int flags);
>   static int __iommu_group_set_domain_internal(struct iommu_group *group,
>   					     struct iommu_domain *new_domain,
>   					     unsigned int flags);
> @@ -405,18 +409,10 @@ static bool iommu_is_attach_deferred(struct device *dev)
>   	return false;
>   }
>   
> -static int iommu_group_do_dma_first_attach(struct device *dev, void *data)
> +static int iommu_group_do_dma_first_attach(struct iommu_group *group, struct device *dev)
>   {
> -	struct iommu_domain *domain = data;
> -
> -	lockdep_assert_held(&dev->iommu_group->mutex);
> -
> -	if (iommu_is_attach_deferred(dev)) {
> -		dev->iommu->attach_deferred = 1;
> -		return 0;
> -	}
> -
> -	return __iommu_attach_device(domain, dev);
> +	return __iommu_device_set_domain(group, dev, group->domain,
> +					 IOMMU_SET_DOMAIN_WITH_DEFERRED);

Just out of curiosity, why is this group->domain instead of
group->default_domain?

>   }
>   
>   int iommu_probe_device(struct device *dev)
> @@ -449,7 +445,7 @@ int iommu_probe_device(struct device *dev)
>   	 * attach the default domain.
>   	 */
>   	if (group->default_domain && !group->owner) {
> -		ret = iommu_group_do_dma_first_attach(dev, group->default_domain);
> +		ret = iommu_group_do_dma_first_attach(group, dev);
>   		if (ret) {
>   			mutex_unlock(&group->mutex);
>   			iommu_group_put(group);
> @@ -1117,7 +1113,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>   	mutex_lock(&group->mutex);
>   	list_add_tail(&device->list, &group->devices);
>   	if (group->domain)
> -		ret = iommu_group_do_dma_first_attach(dev, group->domain);
> +		ret = iommu_group_do_dma_first_attach(group, dev);

I am wondering whether we need to do dma first attach here.

iommu_group_add_device() is called from

1) drivers/vfio/group.c
    The deferred attachment is only used for kdump. In the kdump capture
    kernel, vfio should not be used. Or not?

2) arch/powerpc/kernel/iommu.c
    Is powerpc iommu implementation compatible with the generic IOMMU
    framework and provide a is_attach_deferred op? If not, no need for
    dma first attach either.

If above are true, perhaps we could simply call
__iommu_group_set_domain() here?

>   	mutex_unlock(&group->mutex);
>   	if (ret)
>   		goto err_put_group;

Best regards,
baolu

Re: [PATCH v2 07/14] iommu: Make iommu_group_do_dma_first_attach() simpler

[Jason Gunthorpe]

On Thu, Mar 30, 2023 at 02:42:20PM +0800, Baolu Lu wrote:

> > @@ -405,18 +409,10 @@ static bool iommu_is_attach_deferred(struct device *dev)
> >   	return false;
> >   }
> > -static int iommu_group_do_dma_first_attach(struct device *dev, void *data)
> > +static int iommu_group_do_dma_first_attach(struct iommu_group *group, struct device *dev)
> >   {
> > -	struct iommu_domain *domain = data;
> > -
> > -	lockdep_assert_held(&dev->iommu_group->mutex);
> > -
> > -	if (iommu_is_attach_deferred(dev)) {
> > -		dev->iommu->attach_deferred = 1;
> > -		return 0;
> > -	}
> > -
> > -	return __iommu_attach_device(domain, dev);
> > +	return __iommu_device_set_domain(group, dev, group->domain,
> > +					 IOMMU_SET_DOMAIN_WITH_DEFERRED);
> 
> Just out of curiosity, why is this group->domain instead of
> group->default_domain?

The overall invariant is that all devices are attached to
group->domain, whatever it is.

Devices are not attached to the default_domain, the default_domain is
made the current group->domain which changes all the devices.

Eventually the caller is made to look like:

        if (group->domain) {
                ret = iommu_group_do_dma_first_attach(group, dev);
                if (ret)
                        goto err_unlock;
                iommu_create_device_direct_mappings(group->domain, dev);
        } else if (!group->default_domain) {
                ret = iommu_setup_default_domain(group, 0);
                if (ret)
                        goto err_unlock;
        }

Which makes it clear we can't assume that default_domain == domain at
this point.

> > @@ -449,7 +445,7 @@ int iommu_probe_device(struct device *dev)
> >   	 * attach the default domain.
> >   	 */
> >   	if (group->default_domain && !group->owner) {
> > -		ret = iommu_group_do_dma_first_attach(dev, group->default_domain);
> > +		ret = iommu_group_do_dma_first_attach(group, dev);
> >   		if (ret) {
> >   			mutex_unlock(&group->mutex);
> >   			iommu_group_put(group);
> > @@ -1117,7 +1113,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
> >   	mutex_lock(&group->mutex);
> >   	list_add_tail(&device->list, &group->devices);
> >   	if (group->domain)
> > -		ret = iommu_group_do_dma_first_attach(dev, group->domain);
> > +		ret = iommu_group_do_dma_first_attach(group, dev);
> 
> I am wondering whether we need to do dma first attach here.

Correct, this is wonky - I fix this in the followup series.

Jason

Re: [PATCH v2 07/14] iommu: Make iommu_group_do_dma_first_attach() simpler

[Baolu Lu]

On 3/30/23 10:41 PM, Jason Gunthorpe wrote:
> On Thu, Mar 30, 2023 at 02:42:20PM +0800, Baolu Lu wrote:
> 
>>> @@ -405,18 +409,10 @@ static bool iommu_is_attach_deferred(struct device *dev)
>>>    	return false;
>>>    }
>>> -static int iommu_group_do_dma_first_attach(struct device *dev, void *data)
>>> +static int iommu_group_do_dma_first_attach(struct iommu_group *group, struct device *dev)
>>>    {
>>> -	struct iommu_domain *domain = data;
>>> -
>>> -	lockdep_assert_held(&dev->iommu_group->mutex);
>>> -
>>> -	if (iommu_is_attach_deferred(dev)) {
>>> -		dev->iommu->attach_deferred = 1;
>>> -		return 0;
>>> -	}
>>> -
>>> -	return __iommu_attach_device(domain, dev);
>>> +	return __iommu_device_set_domain(group, dev, group->domain,
>>> +					 IOMMU_SET_DOMAIN_WITH_DEFERRED);
>> Just out of curiosity, why is this group->domain instead of
>> group->default_domain?
> The overall invariant is that all devices are attached to
> group->domain, whatever it is.
> 
> Devices are not attached to the default_domain, the default_domain is
> made the current group->domain which changes all the devices.
> 
> Eventually the caller is made to look like:
> 
>          if (group->domain) {
>                  ret = iommu_group_do_dma_first_attach(group, dev);
>                  if (ret)
>                          goto err_unlock;
>                  iommu_create_device_direct_mappings(group->domain, dev);
>          } else if (!group->default_domain) {
>                  ret = iommu_setup_default_domain(group, 0);
>                  if (ret)
>                          goto err_unlock;
>          }
> 
> Which makes it clear we can't assume that default_domain == domain at
> this point.

Yeah! That's clearer.

Best regards,
baolu

RE: [PATCH v2 07/14] iommu: Make iommu_group_do_dma_first_attach() simpler

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, March 30, 2023 7:41 AM
> 
> It should always attach to the current group->domain, so don't take in a
> domain parameter. Use the __iommu_device_set_domain() common code to
> handle the attach.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

[PATCH v2 08/14] iommu: Make iommu_group_do_dma_first_attach() work with owned groups

[Jason Gunthorpe]

If the group is already owned then defered attach should not be done and
the device should be directly connected to the correct domain.

Owned means that some driver is already bound to devices in the group and
is operating the group possibly without DMA API support. In this case
there would be no way to correct the mismatched domain.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c68cf551d05dc4..6fe8bc78db2016 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -411,8 +411,9 @@ static bool iommu_is_attach_deferred(struct device *dev)
 
 static int iommu_group_do_dma_first_attach(struct iommu_group *group, struct device *dev)
 {
-	return __iommu_device_set_domain(group, dev, group->domain,
-					 IOMMU_SET_DOMAIN_WITH_DEFERRED);
+	return __iommu_device_set_domain(
+		group, dev, group->domain,
+		group->owner ? 0 : IOMMU_SET_DOMAIN_WITH_DEFERRED);
 }
 
 int iommu_probe_device(struct device *dev)
-- 
2.40.0

Re: [PATCH v2 08/14] iommu: Make iommu_group_do_dma_first_attach() work with owned groups

[Baolu Lu]

On 3/30/23 7:40 AM, Jason Gunthorpe wrote:
> If the group is already owned then defered attach should not be done and
> the device should be directly connected to the correct domain.
> 
> Owned means that some driver is already bound to devices in the group and
> is operating the group possibly without DMA API support. In this case
> there would be no way to correct the mismatched domain.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 08/14] iommu: Make iommu_group_do_dma_first_attach() work with owned groups

[Robin Murphy]

On 2023-03-30 00:40, Jason Gunthorpe wrote:
> If the group is already owned then defered attach should not be done and
> the device should be directly connected to the correct domain.
> 
> Owned means that some driver is already bound to devices in the group and
> is operating the group possibly without DMA API support. In this case
> there would be no way to correct the mismatched domain.

How much do we really care about supporting device hotplug while using 
VFIO in a kdump kernel? Surely if anything ever deserved a "don't do 
that"...

In fact I struggle to imagine how it would even be possible for a device 
which *was* present and active when the previous kernel crashed, and 
thus needs deferred attach, to somehow evade bus_iommu_probe() in the 
kdump kernel until after the rest of its group has managed to get as far 
as binding a driver to claim ownership :/

And even if someone did manage to achieve all of this, is it really 
beneficial to let this device start spamming IOMMU faults and/or 
potentially corrupting whatever the kdump kernel *is* trying to do in 
said owned domain, on the offchance that the user might eventually bind 
it to VFIO (or whatever) as well and expect it to then work? TBH I'd say 
that banishing it to roam free in deferred-attach limbo, peacefully 
grazing the wastelands of dead-kernel memory and failing to support any 
non-DMA-API funny business, is the preferable option.

Thanks,
Robin.

> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/iommu.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index c68cf551d05dc4..6fe8bc78db2016 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -411,8 +411,9 @@ static bool iommu_is_attach_deferred(struct device *dev)
>   
>   static int iommu_group_do_dma_first_attach(struct iommu_group *group, struct device *dev)
>   {
> -	return __iommu_device_set_domain(group, dev, group->domain,
> -					 IOMMU_SET_DOMAIN_WITH_DEFERRED);
> +	return __iommu_device_set_domain(
> +		group, dev, group->domain,
> +		group->owner ? 0 : IOMMU_SET_DOMAIN_WITH_DEFERRED);
>   }
>   
>   int iommu_probe_device(struct device *dev)

Re: [PATCH v2 08/14] iommu: Make iommu_group_do_dma_first_attach() work with owned groups

[Jason Gunthorpe]

On Thu, Mar 30, 2023 at 04:54:27PM +0100, Robin Murphy wrote:

> In fact I struggle to imagine how it would even be possible for a device
> which *was* present and active when the previous kernel crashed, and thus
> needs deferred attach, to somehow evade bus_iommu_probe() in the kdump
> kernel until after the rest of its group has managed to get as far as
> binding a driver to claim ownership :/

At least the Intel driver seems to use some kind of global flag
VTD_FLAG_TRANS_PRE_ENABLED. So presumably if I, say, get into a kdump
kernel and hotplug a thunderbolt dock I will see a hotplugged device
with is_attach_deferred = true?

> And even if someone did manage to achieve all of this, is it really
> beneficial to let this device start spamming IOMMU faults and/or potentially
> corrupting whatever the kdump kernel *is* 

If it is a freshly hot plugged device it will be idle with DMA
disabled in the config space.

If it is a pre-existing device that is already running it should have
already been handled, like you say.

At some point after boot the kdump kernel is no longer special and it
is just a normal kernel. It should be behave the same on hotplug as a
non-kdump kernel.

Regardless of the lack of utility, it is logically wrong, the code
reads wrong, it is easy to fix, and it doesn't harm any sane system
that never has owned set in the kdump.

Jason

RE: [PATCH v2 08/14] iommu: Make iommu_group_do_dma_first_attach() work with owned groups

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, March 30, 2023 7:41 AM
> 
> If the group is already owned then defered attach should not be done and
> the device should be directly connected to the correct domain.
> 
> Owned means that some driver is already bound to devices in the group and
> is operating the group possibly without DMA API support. In this case
> there would be no way to correct the mismatched domain.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

[PATCH v2 09/14] iommu: Fix iommu_probe_device() to attach the right domain

[Jason Gunthorpe]

The general invariant is that all devices in an iommu_group are attached
to group->domain. We missed some cases here where an owned group would not
get the device attached.

Rework this logic so it follows the default domain flow of the
bus_iommu_probe() - call iommu_alloc_default_domain(), then use
__iommu_group_set_domain_internal() to set up all the devices.

Finally always attach the device to the current domain if it is already
set.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 45 ++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6fe8bc78db2016..5bbd667f727605 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -432,27 +432,32 @@ 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);
 
-	/*
-	 * If device joined an existing group which has been claimed, don't
-	 * attach the default domain.
-	 */
-	if (group->default_domain && !group->owner) {
+	if (group->domain) {
 		ret = iommu_group_do_dma_first_attach(group, dev);
-		if (ret) {
-			mutex_unlock(&group->mutex);
-			iommu_group_put(group);
-			goto err_release;
-		}
+	} else if (!group->default_domain) {
+		/*
+		 * 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.
+		 */
+		iommu_alloc_default_domain(group, dev);
+		group->domain = NULL;
+		if (group->default_domain)
+			ret = __iommu_group_set_domain_internal(
+				group, group->default_domain,
+				IOMMU_SET_DOMAIN_WITH_DEFERRED);
+
+		/*
+		 * We assume that the iommu driver starts up the device in
+		 * 'set_platform_dma_ops' mode if it does not support default
+		 * domains.
+		 */
 	}
+	if (ret)
+		goto err_unlock;
 
 	iommu_create_device_direct_mappings(group, dev);
 
@@ -465,6 +470,9 @@ int iommu_probe_device(struct device *dev)
 
 	return 0;
 
+err_unlock:
+	mutex_unlock(&group->mutex);
+	iommu_group_put(group);
 err_release:
 	iommu_release_device(dev);
 
@@ -1717,9 +1725,6 @@ static int iommu_alloc_default_domain(struct iommu_group *group,
 {
 	unsigned int type;
 
-	if (group->default_domain)
-		return 0;
-
 	type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type;
 
 	return iommu_group_alloc_default_domain(dev->bus, group, type);
-- 
2.40.0

Re: [PATCH v2 09/14] iommu: Fix iommu_probe_device() to attach the right domain

[Baolu Lu]

On 2023/3/30 7:40, Jason Gunthorpe wrote:
> The general invariant is that all devices in an iommu_group are attached
> to group->domain. We missed some cases here where an owned group would not
> get the device attached.
> 
> Rework this logic so it follows the default domain flow of the
> bus_iommu_probe() - call iommu_alloc_default_domain(), then use
> __iommu_group_set_domain_internal() to set up all the devices.
> 
> Finally always attach the device to the current domain if it is already
> set.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

RE: [PATCH v2 09/14] iommu: Fix iommu_probe_device() to attach the right domain

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, March 30, 2023 7:41 AM
> 
> The general invariant is that all devices in an iommu_group are attached
> to group->domain. We missed some cases here where an owned group
> would not
> get the device attached.
> 
> Rework this logic so it follows the default domain flow of the
> bus_iommu_probe() - call iommu_alloc_default_domain(), then use
> __iommu_group_set_domain_internal() to set up all the devices.
> 
> Finally always attach the device to the current domain if it is already
> set.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

[PATCH v2 10/14] iommu: Remove the assignment of group->domain during default domain alloc

[Jason Gunthorpe]

group->domain should only be set once all the device's drivers have
had their ops->attach_dev() called. iommu_group_alloc_default_domain()
doesn't do this, so it shouldn't set the value.

The previous patches organized things so that each caller of
iommu_group_alloc_default_domain() follows up with calling
__iommu_group_set_domain_internal() that does set the group->domain.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 5bbd667f727605..0eee54d47757d2 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -444,7 +444,6 @@ int iommu_probe_device(struct device *dev)
 		 * checked.
 		 */
 		iommu_alloc_default_domain(group, dev);
-		group->domain = NULL;
 		if (group->default_domain)
 			ret = __iommu_group_set_domain_internal(
 				group, group->default_domain,
@@ -1715,8 +1714,6 @@ static int iommu_group_alloc_default_domain(struct bus_type *bus,
 		return -ENOMEM;
 
 	group->default_domain = dom;
-	if (!group->domain)
-		group->domain = dom;
 	return 0;
 }
 
@@ -1920,7 +1917,6 @@ int bus_iommu_probe(struct bus_type *bus)
 
 		iommu_group_create_direct_mappings(group);
 
-		group->domain = NULL;
 		ret = __iommu_group_set_domain_internal(
 			group, group->default_domain,
 			IOMMU_SET_DOMAIN_WITH_DEFERRED);
-- 
2.40.0

Re: [PATCH v2 10/14] iommu: Remove the assignment of group->domain during default domain alloc

[Baolu Lu]

On 2023/3/30 7:40, Jason Gunthorpe wrote:
> group->domain should only be set once all the device's drivers have
> had their ops->attach_dev() called. iommu_group_alloc_default_domain()
> doesn't do this, so it shouldn't set the value.
> 
> The previous patches organized things so that each caller of
> iommu_group_alloc_default_domain() follows up with calling
> __iommu_group_set_domain_internal() that does set the group->domain.
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

[PATCH v2 11/14] iommu: Consolidate the code to calculate the target default domain type

[Jason Gunthorpe]

Put all the code to calculate the default domain type into one
function. Make the function able to handle the
iommu_change_dev_def_domain() by taking in the target domain type and
erroring out if the target type isn't reachable.

This makes it really clear that specifying a 0 type during
iommu_change_dev_def_domain() will have the same outcome as the normal
probe path.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 88 +++++++++++++++++--------------------------
 1 file changed, 35 insertions(+), 53 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0eee54d47757d2..79e3fa80d0f6cc 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1809,50 +1809,43 @@ static int iommu_bus_notifier(struct notifier_block *nb,
 	return 0;
 }
 
-struct __group_domain_type {
-	struct device *dev;
-	unsigned int type;
-};
-
-static int probe_get_default_domain_type(struct device *dev, void *data)
+/* A target_type of 0 will select the best domain type and cannot fail */
+static int iommu_get_default_domain_type(struct iommu_group *group,
+					 int target_type)
 {
-	struct __group_domain_type *gtype = data;
-	unsigned int type = iommu_get_def_domain_type(dev);
+	int best_type = target_type;
+	struct group_device *gdev;
+	struct device *last_dev;
 
-	if (type) {
-		if (gtype->type && gtype->type != type) {
-			dev_warn(dev, "Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n",
-				 iommu_domain_type_str(type),
-				 dev_name(gtype->dev),
-				 iommu_domain_type_str(gtype->type));
-			gtype->type = 0;
-		}
+	lockdep_assert_held(&group->mutex);
 
-		if (!gtype->dev) {
-			gtype->dev  = dev;
-			gtype->type = type;
+	for_each_group_device(group, gdev) {
+		unsigned int type = iommu_get_def_domain_type(gdev->dev);
+
+		if (best_type && type && best_type != type) {
+			if (target_type) {
+				dev_err_ratelimited(
+					gdev->dev,
+					"Device cannot be in %s domain\n",
+					iommu_domain_type_str(target_type));
+				return -1;
+			}
+
+			dev_warn(
+				gdev->dev,
+				"Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n",
+				iommu_domain_type_str(type), dev_name(last_dev),
+				iommu_domain_type_str(best_type));
+			best_type = 0;
 		}
+		if (!best_type)
+			best_type = type;
+		last_dev = gdev->dev;
 	}
 
-	return 0;
-}
-
-static void probe_alloc_default_domain(struct bus_type *bus,
-				       struct iommu_group *group)
-{
-	struct __group_domain_type gtype;
-
-	memset(&gtype, 0, sizeof(gtype));
-
-	/* Ask for default domain requirements of all devices in the group */
-	__iommu_group_for_each_dev(group, &gtype,
-				   probe_get_default_domain_type);
-
-	if (!gtype.type)
-		gtype.type = iommu_def_domain_type;
-
-	iommu_group_alloc_default_domain(bus, group, gtype.type);
-
+	if (!best_type)
+		return iommu_def_domain_type;
+	return best_type;
 }
 
 static int iommu_group_do_probe_finalize(struct device *dev, void *data)
@@ -1908,7 +1901,8 @@ int bus_iommu_probe(struct bus_type *bus)
 		list_del_init(&group->entry);
 
 		/* Try to allocate default domain */
-		probe_alloc_default_domain(bus, group);
+		iommu_group_alloc_default_domain(
+			bus, group, iommu_get_default_domain_type(group, 0));
 
 		if (!group->default_domain) {
 			mutex_unlock(&group->mutex);
@@ -2928,27 +2922,15 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
 static int iommu_change_dev_def_domain(struct iommu_group *group,
 				       struct device *dev, int type)
 {
-	struct __group_domain_type gtype = {NULL, 0};
 	struct iommu_domain *prev_dom;
 	int ret;
 
 	lockdep_assert_held(&group->mutex);
 
 	prev_dom = group->default_domain;
-	__iommu_group_for_each_dev(group, &gtype,
-				   probe_get_default_domain_type);
-	if (!type) {
-		/*
-		 * If the user hasn't requested any specific type of domain and
-		 * if the device supports both the domains, then default to the
-		 * domain the device was booted with
-		 */
-		type = gtype.type ? : iommu_def_domain_type;
-	} else if (gtype.type && type != gtype.type) {
-		dev_err_ratelimited(dev, "Device cannot be in %s domain\n",
-				    iommu_domain_type_str(type));
+	type = iommu_get_default_domain_type(group, type);
+	if (type < 0)
 		return -EINVAL;
-	}
 
 	/*
 	 * Switch to a new domain only if the requested domain type is different
-- 
2.40.0

Re: [PATCH v2 11/14] iommu: Consolidate the code to calculate the target default domain type

[Baolu Lu]

On 2023/3/30 7:40, Jason Gunthorpe wrote:
> Put all the code to calculate the default domain type into one
> function. Make the function able to handle the
> iommu_change_dev_def_domain() by taking in the target domain type and
> erroring out if the target type isn't reachable.
> 
> This makes it really clear that specifying a 0 type during
> iommu_change_dev_def_domain() will have the same outcome as the normal
> probe path.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

RE: [PATCH v2 11/14] iommu: Consolidate the code to calculate the target default domain type

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, March 30, 2023 7:41 AM
>
> +	for_each_group_device(group, gdev) {
> +		unsigned int type = iommu_get_def_domain_type(gdev-
> >dev);
> +
> +		if (best_type && type && best_type != type) {
> +			if (target_type) {
> +				dev_err_ratelimited(
> +					gdev->dev,
> +					"Device cannot be in %s domain\n",
> +
> 	iommu_domain_type_str(target_type));
> +				return -1;
> +			}
> +
> +			dev_warn(
> +				gdev->dev,
> +				"Device needs domain type %s, but device %s
> in the same iommu group requires type %s - using default\n",
> +				iommu_domain_type_str(type),
> dev_name(last_dev),
> +				iommu_domain_type_str(best_type));
> +			best_type = 0;

well, in this case best_type is immediately overridden by following lines
then "- using default" in the warning message doesn't hold.

>  		}
> +		if (!best_type)
> +			best_type = type;

Re: [PATCH v2 11/14] iommu: Consolidate the code to calculate the target default domain type

[Jason Gunthorpe]

On Tue, Apr 04, 2023 at 09:39:08AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, March 30, 2023 7:41 AM
> >
> > +	for_each_group_device(group, gdev) {
> > +		unsigned int type = iommu_get_def_domain_type(gdev-
> > >dev);
> > +
> > +		if (best_type && type && best_type != type) {
> > +			if (target_type) {
> > +				dev_err_ratelimited(
> > +					gdev->dev,
> > +					"Device cannot be in %s domain\n",
> > +
> > 	iommu_domain_type_str(target_type));
> > +				return -1;
> > +			}
> > +
> > +			dev_warn(
> > +				gdev->dev,
> > +				"Device needs domain type %s, but device %s
> > in the same iommu group requires type %s - using default\n",
> > +				iommu_domain_type_str(type),
> > dev_name(last_dev),
> > +				iommu_domain_type_str(best_type));
> > +			best_type = 0;
> 
> well, in this case best_type is immediately overridden by following lines
> then "- using default" in the warning message doesn't hold.

Ah... Yes based on the original code this should just be return 0.

It was:

       unsigned int type = iommu_get_def_domain_type(dev);

        if (type) {
                if (gtype->type && gtype->type != type) {
                        dev_warn(dev, "Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n",
                                 iommu_domain_type_str(type),
                                 dev_name(gtype->dev),
                                 iommu_domain_type_str(gtype->type));
                        gtype->type = 0;
                }

                if (!gtype->dev) {
                        gtype->dev  = dev;
                        gtype->type = type;
                }
        }

So if dev is not set then we can't get a type conflict and once dev is
set then it sticks at 0

Thanks,
Jason

[PATCH v2 12/14] iommu: Consolidate the default_domain setup to one function

[Jason Gunthorpe]

The default_domain setup flow requires
 - Determining the default type
 - Allocating a default_domain
 - Attaching it to devices
 - Doing iommu_create_device_direct_mappings()

This is sprinkled around three places written in two different ways. Make
it all the same in one function.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 256 ++++++++++++++++--------------------------
 1 file changed, 96 insertions(+), 160 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 79e3fa80d0f6cc..26129423fa7b95 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -91,8 +91,6 @@ static const char * const iommu_group_resv_type_string[] = {
 
 static int iommu_bus_notifier(struct notifier_block *nb,
 			      unsigned long action, void *data);
-static int iommu_alloc_default_domain(struct iommu_group *group,
-				      struct device *dev);
 static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
 						 unsigned type);
 static int __iommu_attach_device(struct iommu_domain *domain,
@@ -124,7 +122,9 @@ static void __iommu_group_set_domain_nofail(struct iommu_group *group,
 		group, new_domain, IOMMU_SET_DOMAIN_MUST_SUCCEED));
 }
 
-static int iommu_create_device_direct_mappings(struct iommu_group *group,
+static int iommu_setup_default_domain(struct iommu_group *group,
+				      int target_type);
+static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
 					       struct device *dev);
 static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
 static ssize_t iommu_group_store_type(struct iommu_group *group,
@@ -436,29 +436,14 @@ int iommu_probe_device(struct device *dev)
 
 	if (group->domain) {
 		ret = iommu_group_do_dma_first_attach(group, dev);
+		if (ret)
+			goto err_unlock;
+		iommu_create_device_direct_mappings(group->domain, dev);
 	} else if (!group->default_domain) {
-		/*
-		 * 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.
-		 */
-		iommu_alloc_default_domain(group, dev);
-		if (group->default_domain)
-			ret = __iommu_group_set_domain_internal(
-				group, group->default_domain,
-				IOMMU_SET_DOMAIN_WITH_DEFERRED);
-
-		/*
-		 * We assume that the iommu driver starts up the device in
-		 * 'set_platform_dma_ops' mode if it does not support default
-		 * domains.
-		 */
+		ret = iommu_setup_default_domain(group, 0);
+		if (ret)
+			goto err_unlock;
 	}
-	if (ret)
-		goto err_unlock;
-
-	iommu_create_device_direct_mappings(group, dev);
 
 	mutex_unlock(&group->mutex);
 	iommu_group_put(group);
@@ -1004,16 +989,15 @@ int iommu_group_set_name(struct iommu_group *group, const char *name)
 }
 EXPORT_SYMBOL_GPL(iommu_group_set_name);
 
-static int iommu_create_device_direct_mappings(struct iommu_group *group,
+static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
 					       struct device *dev)
 {
-	struct iommu_domain *domain = group->default_domain;
 	struct iommu_resv_region *entry;
 	struct list_head mappings;
 	unsigned long pg_size;
 	int ret = 0;
 
-	if (!domain || !iommu_is_dma_domain(domain))
+	if (!iommu_is_dma_domain(domain))
 		return 0;
 
 	BUG_ON(!domain->pgsize_bitmap);
@@ -1696,37 +1680,6 @@ static int iommu_get_def_domain_type(struct device *dev)
 	return 0;
 }
 
-static int iommu_group_alloc_default_domain(struct bus_type *bus,
-					    struct iommu_group *group,
-					    unsigned int type)
-{
-	struct iommu_domain *dom;
-
-	dom = __iommu_domain_alloc(bus, type);
-	if (!dom && type != IOMMU_DOMAIN_DMA) {
-		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
-		if (dom)
-			pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
-				type, group->name);
-	}
-
-	if (!dom)
-		return -ENOMEM;
-
-	group->default_domain = dom;
-	return 0;
-}
-
-static int iommu_alloc_default_domain(struct iommu_group *group,
-				      struct device *dev)
-{
-	unsigned int type;
-
-	type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type;
-
-	return iommu_group_alloc_default_domain(dev->bus, group, type);
-}
-
 /**
  * iommu_group_get_for_dev - Find or create the IOMMU group for a device
  * @dev: target device
@@ -1848,6 +1801,84 @@ static int iommu_get_default_domain_type(struct iommu_group *group,
 	return best_type;
 }
 
+/**
+ * iommu_setup_default_domain - Set the default_domain for the group
+ * @group: Group to change
+ * @target_type: Domain type to set as the default_domain
+ *
+ * Allocate a default domain and set it as the current domain on the group. If
+ * the group already has a default domain it will be changed to the target_type.
+ * When target_type is 0 the default domain is selected based on driver and
+ * system preferences.
+ */
+static int iommu_setup_default_domain(struct iommu_group *group,
+				      int target_type)
+{
+	struct group_device *gdev;
+	struct iommu_domain *dom;
+	struct bus_type *bus =
+		list_first_entry(&group->devices, struct group_device, list)
+			->dev->bus;
+	int ret;
+
+	lockdep_assert_held(&group->mutex);
+
+	target_type = iommu_get_default_domain_type(group, target_type);
+	if (target_type < 0)
+		return -EINVAL;
+
+	if (group->default_domain && group->default_domain->type == target_type)
+		return 0;
+
+	dom = __iommu_domain_alloc(bus, target_type);
+	if (!dom && target_type != IOMMU_DOMAIN_DMA) {
+		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
+		if (dom)
+			pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
+				target_type, group->name);
+	}
+
+	/*
+	 * There are still some drivers which don't support default domains, so
+	 * we ignore the failure and leave group->default_domain NULL.
+	 *
+	 * We assume that the iommu driver starts up the device in
+	 * 'set_platform_dma_ops' mode if it does not support default domains.
+	 */
+	if (!dom) {
+		ret = 0;
+		goto out_set;
+	}
+
+	ret = __iommu_group_set_domain_internal(group, dom,
+						IOMMU_SET_DOMAIN_WITH_DEFERRED);
+	if (ret) {
+		/*
+		 * An attach_dev failure may result in some devices being left
+		 * attached to dom. This is not cleaned up until release_device
+		 * is called. Thus we can't always free dom on failure, we have
+		 * no choice but to stick the broken domain into
+		 * group->default_domain to defer the free and try to continue.
+		 */
+		if (list_count_nodes(&group->devices) > 1)
+			goto out_set;
+
+		iommu_domain_free(dom);
+		dom = NULL;
+		goto out_set;
+	}
+
+	/* The domain must be attached before we can establish any mappings */
+	for_each_group_device(group, gdev)
+		iommu_create_device_direct_mappings(dom, gdev->dev);
+
+out_set:
+	if (group->default_domain)
+		iommu_domain_free(group->default_domain);
+	group->default_domain = dom;
+	return ret;
+}
+
 static int iommu_group_do_probe_finalize(struct device *dev, void *data)
 {
 	const struct iommu_ops *ops = dev_iommu_ops(dev);
@@ -1864,21 +1895,6 @@ static void __iommu_group_dma_finalize(struct iommu_group *group)
 				   iommu_group_do_probe_finalize);
 }
 
-static int iommu_do_create_direct_mappings(struct device *dev, void *data)
-{
-	struct iommu_group *group = data;
-
-	iommu_create_device_direct_mappings(group, dev);
-
-	return 0;
-}
-
-static int iommu_group_create_direct_mappings(struct iommu_group *group)
-{
-	return __iommu_group_for_each_dev(group, group,
-					  iommu_do_create_direct_mappings);
-}
-
 int bus_iommu_probe(struct bus_type *bus)
 {
 	struct iommu_group *group, *next;
@@ -1900,30 +1916,18 @@ int bus_iommu_probe(struct bus_type *bus)
 		/* Remove item from the list */
 		list_del_init(&group->entry);
 
-		/* Try to allocate default domain */
-		iommu_group_alloc_default_domain(
-			bus, group, iommu_get_default_domain_type(group, 0));
-
 		if (!group->default_domain) {
-			mutex_unlock(&group->mutex);
-			continue;
+			ret = iommu_setup_default_domain(group, 0);
+			if (ret) {
+				mutex_unlock(&group->mutex);
+				return ret;
+			}
 		}
-
-		iommu_group_create_direct_mappings(group);
-
-		ret = __iommu_group_set_domain_internal(
-			group, group->default_domain,
-			IOMMU_SET_DOMAIN_WITH_DEFERRED);
-
 		mutex_unlock(&group->mutex);
-
-		if (ret)
-			break;
-
 		__iommu_group_dma_finalize(group);
 	}
 
-	return ret;
+	return 0;
 }
 
 bool iommu_present(struct bus_type *bus)
@@ -2905,69 +2909,6 @@ int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
 }
 EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
 
-/*
- * Changes the default domain of an iommu group
- *
- * @group: The group for which the default domain should be changed
- * @dev: The first device in the group
- * @type: The type of the new default domain that gets associated with the group
- *
- * Returns 0 on success and error code on failure
- *
- * Note:
- * 1. Presently, this function is called only when user requests to change the
- *    group's default domain type through /sys/kernel/iommu_groups/<grp_id>/type
- *    Please take a closer look if intended to use for other purposes.
- */
-static int iommu_change_dev_def_domain(struct iommu_group *group,
-				       struct device *dev, int type)
-{
-	struct iommu_domain *prev_dom;
-	int ret;
-
-	lockdep_assert_held(&group->mutex);
-
-	prev_dom = group->default_domain;
-	type = iommu_get_default_domain_type(group, type);
-	if (type < 0)
-		return -EINVAL;
-
-	/*
-	 * Switch to a new domain only if the requested domain type is different
-	 * from the existing default domain type
-	 */
-	if (prev_dom->type == type)
-		return 0;
-
-	group->default_domain = NULL;
-	group->domain = NULL;
-
-	/* Sets group->default_domain to the newly allocated domain */
-	ret = iommu_group_alloc_default_domain(dev->bus, group, type);
-	if (ret)
-		goto restore_old_domain;
-
-	group->domain = prev_dom;
-	ret = iommu_create_device_direct_mappings(group, dev);
-	if (ret)
-		goto free_new_domain;
-
-	ret = __iommu_group_set_domain(group, group->default_domain);
-	if (ret)
-		goto free_new_domain;
-
-	iommu_domain_free(prev_dom);
-
-	return 0;
-
-free_new_domain:
-	iommu_domain_free(group->default_domain);
-restore_old_domain:
-	group->default_domain = prev_dom;
-
-	return ret;
-}
-
 /*
  * Changing the default domain through sysfs requires the users to unbind the
  * drivers from the devices in the iommu group, except for a DMA -> DMA-FQ
@@ -2980,8 +2921,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 static ssize_t iommu_group_store_type(struct iommu_group *group,
 				      const char *buf, size_t count)
 {
-	struct group_device *grp_dev;
-	struct device *dev;
 	int ret, req_type;
 
 	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
@@ -3019,10 +2958,7 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 		return -EPERM;
 	}
 
-	grp_dev = list_first_entry(&group->devices, struct group_device, list);
-	dev = grp_dev->dev;
-
-	ret = iommu_change_dev_def_domain(group, dev, req_type);
+	ret = iommu_setup_default_domain(group, req_type);
 
 	/*
 	 * Release the mutex here because ops->probe_finalize() call-back of
-- 
2.40.0

Re: [PATCH v2 12/14] iommu: Consolidate the default_domain setup to one function

[Baolu Lu]

On 2023/3/30 7:40, Jason Gunthorpe wrote:
> +/**
> + * iommu_setup_default_domain - Set the default_domain for the group
> + * @group: Group to change
> + * @target_type: Domain type to set as the default_domain
> + *
> + * Allocate a default domain and set it as the current domain on the group. If
> + * the group already has a default domain it will be changed to the target_type.
> + * When target_type is 0 the default domain is selected based on driver and
> + * system preferences.
> + */
> +static int iommu_setup_default_domain(struct iommu_group *group,
> +				      int target_type)
> +{
> +	struct group_device *gdev;
> +	struct iommu_domain *dom;
> +	struct bus_type *bus =
> +		list_first_entry(&group->devices, struct group_device, list)
> +			->dev->bus;
> +	int ret;
> +
> +	lockdep_assert_held(&group->mutex);
> +
> +	target_type = iommu_get_default_domain_type(group, target_type);
> +	if (target_type < 0)
> +		return -EINVAL;
> +
> +	if (group->default_domain && group->default_domain->type == target_type)
> +		return 0;
> +
> +	dom = __iommu_domain_alloc(bus, target_type);
> +	if (!dom && target_type != IOMMU_DOMAIN_DMA) {
> +		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
> +		if (dom)
> +			pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
> +				target_type, group->name);
> +	}

The background of the code above is that some ARM IOMMU drivers only
support DMA mapping domain and do not support identity domain.
Therefore, during boot, if the allocation of identity domain fails, a
DMA mapping domain is used instead.

However, this does not apply to use cases that change the default domain
through sysfs. In such cases, it seems that we should directly return
failure (-ENODEV) and tell the user that the iommu driver does not
support identity domain.

> +
> +	/*
> +	 * There are still some drivers which don't support default domains, so
> +	 * we ignore the failure and leave group->default_domain NULL.
> +	 *
> +	 * We assume that the iommu driver starts up the device in
> +	 * 'set_platform_dma_ops' mode if it does not support default domains.
> +	 */
> +	if (!dom) {
> +		ret = 0;
> +		goto out_set;
> +	}

Should we call set_platform_dma_ops here? The existing default domain
(if exists) will be freed below. But the iommu driver doesn't know about
this. It probably will create a UAF case?

> +
> +	ret = __iommu_group_set_domain_internal(group, dom,
> +						IOMMU_SET_DOMAIN_WITH_DEFERRED);
> +	if (ret) {
> +		/*
> +		 * An attach_dev failure may result in some devices being left
> +		 * attached to dom. This is not cleaned up until release_device
> +		 * is called. Thus we can't always free dom on failure, we have
> +		 * no choice but to stick the broken domain into
> +		 * group->default_domain to defer the free and try to continue.
> +		 */
> +		if (list_count_nodes(&group->devices) > 1)
> +			goto out_set;
> +
> +		iommu_domain_free(dom);
> +		dom = NULL;
> +		goto out_set;
> +	}
> +
> +	/* The domain must be attached before we can establish any mappings */
> +	for_each_group_device(group, gdev)
> +		iommu_create_device_direct_mappings(dom, gdev->dev);

It's better to move creating direct mappings before setting the domain
to the group devices.

The VT-d platforms allow the firmware to access the memory regions
defined in RMRR ACPI table. If we set an empty domain to the device
while the firmware DMA accesses the RMRR memory, it might result in
spurious DMA faults.

> +
> +out_set:
> +	if (group->default_domain)
> +		iommu_domain_free(group->default_domain);
> +	group->default_domain = dom;
> +	return ret;
> +}

Best regards,
baolu

Re: [PATCH v2 12/14] iommu: Consolidate the default_domain setup to one function

[Robin Murphy]

On 2023-03-30 13:37, Baolu Lu wrote:
> On 2023/3/30 7:40, Jason Gunthorpe wrote:
>> +/**
>> + * iommu_setup_default_domain - Set the default_domain for the group
>> + * @group: Group to change
>> + * @target_type: Domain type to set as the default_domain
>> + *
>> + * Allocate a default domain and set it as the current domain on the 
>> group. If
>> + * the group already has a default domain it will be changed to the 
>> target_type.
>> + * When target_type is 0 the default domain is selected based on 
>> driver and
>> + * system preferences.
>> + */
>> +static int iommu_setup_default_domain(struct iommu_group *group,
>> +                      int target_type)
>> +{
>> +    struct group_device *gdev;
>> +    struct iommu_domain *dom;
>> +    struct bus_type *bus =
>> +        list_first_entry(&group->devices, struct group_device, list)
>> +            ->dev->bus;
>> +    int ret;
>> +
>> +    lockdep_assert_held(&group->mutex);
>> +
>> +    target_type = iommu_get_default_domain_type(group, target_type);
>> +    if (target_type < 0)
>> +        return -EINVAL;
>> +
>> +    if (group->default_domain && group->default_domain->type == 
>> target_type)
>> +        return 0;
>> +
>> +    dom = __iommu_domain_alloc(bus, target_type);
>> +    if (!dom && target_type != IOMMU_DOMAIN_DMA) {
>> +        dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
>> +        if (dom)
>> +            pr_warn("Failed to allocate default IOMMU domain of type 
>> %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
>> +                target_type, group->name);
>> +    }
> 
> The background of the code above is that some ARM IOMMU drivers only
> support DMA mapping domain and do not support identity domain.
> Therefore, during boot, if the allocation of identity domain fails, a
> DMA mapping domain is used instead.
> 
> However, this does not apply to use cases that change the default domain
> through sysfs. In such cases, it seems that we should directly return
> failure (-ENODEV) and tell the user that the iommu driver does not
> support identity domain.
> 
>> +
>> +    /*
>> +     * There are still some drivers which don't support default 
>> domains, so
>> +     * we ignore the failure and leave group->default_domain NULL.
>> +     *
>> +     * We assume that the iommu driver starts up the device in
>> +     * 'set_platform_dma_ops' mode if it does not support default 
>> domains.
>> +     */
>> +    if (!dom) {
>> +        ret = 0;
>> +        goto out_set;
>> +    }
> 
> Should we call set_platform_dma_ops here? The existing default domain
> (if exists) will be freed below. But the iommu driver doesn't know about
> this. It probably will create a UAF case?
> 
>> +
>> +    ret = __iommu_group_set_domain_internal(group, dom,
>> +                        IOMMU_SET_DOMAIN_WITH_DEFERRED);
>> +    if (ret) {
>> +        /*
>> +         * An attach_dev failure may result in some devices being left
>> +         * attached to dom. This is not cleaned up until release_device
>> +         * is called. Thus we can't always free dom on failure, we have
>> +         * no choice but to stick the broken domain into
>> +         * group->default_domain to defer the free and try to continue.
>> +         */
>> +        if (list_count_nodes(&group->devices) > 1)
>> +            goto out_set;
>> +
>> +        iommu_domain_free(dom);
>> +        dom = NULL;
>> +        goto out_set;
>> +    }
>> +
>> +    /* The domain must be attached before we can establish any 
>> mappings */
>> +    for_each_group_device(group, gdev)
>> +        iommu_create_device_direct_mappings(dom, gdev->dev);
> 
> It's better to move creating direct mappings before setting the domain
> to the group devices.
> 
> The VT-d platforms allow the firmware to access the memory regions
> defined in RMRR ACPI table. If we set an empty domain to the device
> while the firmware DMA accesses the RMRR memory, it might result in
> spurious DMA faults.

Yes, logically IOMMU_RESV_DIRECT and IOMMU_RESV_DIRECT_RELAXABLE regions 
*must* be mapped before their device is attached, in order to guarantee 
continuity. Yes, this means the RMR stuff for Arm SMMU still isn't truly 
working properly - it still needs domain_alloc fixing to get rid of the 
whole horrible "finalise on first attach" idiom, which sadly I haven't 
had time to get back to yet :(

Thanks,
Robin.

> 
>> +
>> +out_set:
>> +    if (group->default_domain)
>> +        iommu_domain_free(group->default_domain);
>> +    group->default_domain = dom;
>> +    return ret;
>> +}
> 
> Best regards,
> baolu

Re: [PATCH v2 12/14] iommu: Consolidate the default_domain setup to one function

[Jason Gunthorpe]

On Thu, Mar 30, 2023 at 03:29:00PM +0100, Robin Murphy wrote:

> > > +    /* The domain must be attached before we can establish any
> > > mappings */
> > > +    for_each_group_device(group, gdev)
> > > +        iommu_create_device_direct_mappings(dom, gdev->dev);
> > 
> > It's better to move creating direct mappings before setting the domain
> > to the group devices.
> > 
> > The VT-d platforms allow the firmware to access the memory regions
> > defined in RMRR ACPI table. If we set an empty domain to the device
> > while the firmware DMA accesses the RMRR memory, it might result in
> > spurious DMA faults.
> 
> Yes, logically IOMMU_RESV_DIRECT and IOMMU_RESV_DIRECT_RELAXABLE regions
> *must* be mapped before their device is attached, in order to guarantee
> continuity. Yes, this means the RMR stuff for Arm SMMU still isn't truly
> working properly - it still needs domain_alloc fixing to get rid of the
> whole horrible "finalise on first attach" idiom, which sadly I haven't had
> time to get back to yet :(

Yes, this is the only reason I put it in this order is because otherwise it
doesn't work at all on ARM and just quiely NOPs itself without any
error reporting. That seemed bad..

IIRC the existing code had both orderings :( I'll add a note about
this justification

Thanks,
Jason

Re: [PATCH v2 12/14] iommu: Consolidate the default_domain setup to one function

[Jason Gunthorpe]

On Thu, Mar 30, 2023 at 11:45:10AM -0300, Jason Gunthorpe wrote:

> IIRC the existing code had both orderings :( I'll add a note about
> this justification

Robin, would this be helpful in the short term for ARM?

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c31c5d6f913194..39c923e71e625a 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1778,6 +1778,7 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 	struct bus_type *bus =
 		list_first_entry(&group->devices, struct group_device, list)
 			->dev->bus;
+	bool direct_failed;
 	int req_type;
 	int ret;
 
@@ -1824,8 +1825,10 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 	 * mapped before their device is attached, in order to guarantee
 	 * continuity with any FW activity
 	 */
+	direct_failed = false;
 	for_each_group_device(group, gdev)
-		iommu_create_device_direct_mappings(dom, gdev->dev);
+		if (iommu_create_device_direct_mappings(dom, gdev->dev))
+			direct_failed = true;
 
 	ret = __iommu_group_set_domain_internal(group, dom,
 						IOMMU_SET_DOMAIN_WITH_DEFERRED);
@@ -1845,6 +1848,16 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 		goto out_set;
 	}
 
+	/*
+	 * Driver's are supposed to allow mappings to be installed in a domain
+	 * before device attachment, but some don't. Hack around this defect by
+	 * trying again after.
+	 */
+	if (direct_failed) {
+		for_each_group_device(group, gdev)
+			iommu_create_device_direct_mappings(dom, gdev->dev);
+	}
+
 out_set:
 	if (group->default_domain)
 		iommu_domain_free(group->default_domain);

Re: [PATCH v2 12/14] iommu: Consolidate the default_domain setup to one function

[Robin Murphy]

On 2023-03-30 16:42, Jason Gunthorpe wrote:
> On Thu, Mar 30, 2023 at 11:45:10AM -0300, Jason Gunthorpe wrote:
> 
>> IIRC the existing code had both orderings :( I'll add a note about
>> this justification
> 
> Robin, would this be helpful in the short term for ARM?

Yeah, something along these lines seems like the neatest compromise for now.

Cheers,
Robin.

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index c31c5d6f913194..39c923e71e625a 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1778,6 +1778,7 @@ static int iommu_setup_default_domain(struct iommu_group *group,
>   	struct bus_type *bus =
>   		list_first_entry(&group->devices, struct group_device, list)
>   			->dev->bus;
> +	bool direct_failed;
>   	int req_type;
>   	int ret;
>   
> @@ -1824,8 +1825,10 @@ static int iommu_setup_default_domain(struct iommu_group *group,
>   	 * mapped before their device is attached, in order to guarantee
>   	 * continuity with any FW activity
>   	 */
> +	direct_failed = false;
>   	for_each_group_device(group, gdev)
> -		iommu_create_device_direct_mappings(dom, gdev->dev);
> +		if (iommu_create_device_direct_mappings(dom, gdev->dev))
> +			direct_failed = true;
>   
>   	ret = __iommu_group_set_domain_internal(group, dom,
>   						IOMMU_SET_DOMAIN_WITH_DEFERRED);
> @@ -1845,6 +1848,16 @@ static int iommu_setup_default_domain(struct iommu_group *group,
>   		goto out_set;
>   	}
>   
> +	/*
> +	 * Driver's are supposed to allow mappings to be installed in a domain
> +	 * before device attachment, but some don't. Hack around this defect by
> +	 * trying again after.
> +	 */
> +	if (direct_failed) {
> +		for_each_group_device(group, gdev)
> +			iommu_create_device_direct_mappings(dom, gdev->dev);
> +	}
> +
>   out_set:
>   	if (group->default_domain)
>   		iommu_domain_free(group->default_domain);

Re: [PATCH v2 12/14] iommu: Consolidate the default_domain setup to one function

[Jason Gunthorpe]

On Thu, Mar 30, 2023 at 08:37:16PM +0800, Baolu Lu wrote:
> On 2023/3/30 7:40, Jason Gunthorpe wrote:
> > +/**
> > + * iommu_setup_default_domain - Set the default_domain for the group
> > + * @group: Group to change
> > + * @target_type: Domain type to set as the default_domain
> > + *
> > + * Allocate a default domain and set it as the current domain on the group. If
> > + * the group already has a default domain it will be changed to the target_type.
> > + * When target_type is 0 the default domain is selected based on driver and
> > + * system preferences.
> > + */
> > +static int iommu_setup_default_domain(struct iommu_group *group,
> > +				      int target_type)
> > +{
> > +	struct group_device *gdev;
> > +	struct iommu_domain *dom;
> > +	struct bus_type *bus =
> > +		list_first_entry(&group->devices, struct group_device, list)
> > +			->dev->bus;
> > +	int ret;
> > +
> > +	lockdep_assert_held(&group->mutex);
> > +
> > +	target_type = iommu_get_default_domain_type(group, target_type);
> > +	if (target_type < 0)
> > +		return -EINVAL;
> > +
> > +	if (group->default_domain && group->default_domain->type == target_type)
> > +		return 0;
> > +
> > +	dom = __iommu_domain_alloc(bus, target_type);
> > +	if (!dom && target_type != IOMMU_DOMAIN_DMA) {
> > +		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
> > +		if (dom)
> > +			pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
> > +				target_type, group->name);
> > +	}
> 
> The background of the code above is that some ARM IOMMU drivers only
> support DMA mapping domain and do not support identity domain.
> Therefore, during boot, if the allocation of identity domain fails, a
> DMA mapping domain is used instead.

Er, this is doing two things then because it also allows DMA_FQ to
degrade to just DMA..

I changed it like this:

	dom = __iommu_domain_alloc(bus, req_type);
	if (!dom && !target_type &&
	    (req_type == IOMMU_DOMAIN_IDENTITY ||
	     req_type == IOMMU_DOMAIN_DMA_FQ)) {
		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);

So the auto selection only happens if the target_type is not automatic.
 
> However, this does not apply to use cases that change the default domain
> through sysfs. In such cases, it seems that we should directly return
> failure (-ENODEV) and tell the user that the iommu driver does not
> support identity domain.

And with the fix below it will return ENODEV rather than autoselect DMA.

> > +	/*
> > +	 * There are still some drivers which don't support default domains, so
> > +	 * we ignore the failure and leave group->default_domain NULL.
> > +	 *
> > +	 * We assume that the iommu driver starts up the device in
> > +	 * 'set_platform_dma_ops' mode if it does not support default domains.
> > +	 */
> > +	if (!dom) {
> > +		ret = 0;
> > +		goto out_set;
> > +	}
> 
> Should we call set_platform_dma_ops here?

It could be done and should be harmless, but the driver is supposed to
start up in that mode so don't need to explicitly enter it when
plugging the device.. I kept things as-is

> The existing default domain
> (if exists) will be freed below. But the iommu driver doesn't know about
> this. It probably will create a UAF case?

This is a bug:

	if (!dom) {
		/* Once in default_domain mode we never leave */
		if (group->default_domain)
			return -ENODEV;

So default_domain is either NULL forever or set to something forever.

Jason

Re: [PATCH v2 12/14] iommu: Consolidate the default_domain setup to one function

[Robin Murphy]

On 2023-03-30 16:36, Jason Gunthorpe wrote:
> On Thu, Mar 30, 2023 at 08:37:16PM +0800, Baolu Lu wrote:
>> On 2023/3/30 7:40, Jason Gunthorpe wrote:
>>> +/**
>>> + * iommu_setup_default_domain - Set the default_domain for the group
>>> + * @group: Group to change
>>> + * @target_type: Domain type to set as the default_domain
>>> + *
>>> + * Allocate a default domain and set it as the current domain on the group. If
>>> + * the group already has a default domain it will be changed to the target_type.
>>> + * When target_type is 0 the default domain is selected based on driver and
>>> + * system preferences.
>>> + */
>>> +static int iommu_setup_default_domain(struct iommu_group *group,
>>> +				      int target_type)
>>> +{
>>> +	struct group_device *gdev;
>>> +	struct iommu_domain *dom;
>>> +	struct bus_type *bus =
>>> +		list_first_entry(&group->devices, struct group_device, list)
>>> +			->dev->bus;
>>> +	int ret;
>>> +
>>> +	lockdep_assert_held(&group->mutex);
>>> +
>>> +	target_type = iommu_get_default_domain_type(group, target_type);
>>> +	if (target_type < 0)
>>> +		return -EINVAL;
>>> +
>>> +	if (group->default_domain && group->default_domain->type == target_type)
>>> +		return 0;
>>> +
>>> +	dom = __iommu_domain_alloc(bus, target_type);
>>> +	if (!dom && target_type != IOMMU_DOMAIN_DMA) {
>>> +		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
>>> +		if (dom)
>>> +			pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
>>> +				target_type, group->name);
>>> +	}
>>
>> The background of the code above is that some ARM IOMMU drivers only
>> support DMA mapping domain and do not support identity domain.
>> Therefore, during boot, if the allocation of identity domain fails, a
>> DMA mapping domain is used instead.
> 
> Er, this is doing two things then because it also allows DMA_FQ to
> degrade to just DMA..

Same thing really; generally the point is that any of the more "special" 
default domain types set with the broad brush of Kconfig or command-line 
can cleanly fall back to the basic type wherever it turns out that 
there's a less-capable driver (or, eventually, IOMMU instance) present.

> I changed it like this:
> 
> 	dom = __iommu_domain_alloc(bus, req_type);
> 	if (!dom && !target_type &&
> 	    (req_type == IOMMU_DOMAIN_IDENTITY ||
> 	     req_type == IOMMU_DOMAIN_DMA_FQ)) {
> 		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
> 
> So the auto selection only happens if the target_type is not automatic.

You mean "is automatic"? I'm missing how target_type and req_type are 
related here, but the general principle should be that if we have 
derived the type from iommu_def_domain_type, because we had nothing more 
specific from either a sysfs request or iommu_get_def_domain_type(), 
then a !IOMMU_DOMAIN_DMA failure can retry with IOMMU_DOMAIN_DMA. 
Otherwise any failure should be final.

Strictly that's not quite what the current code does, since 
iommu_alloc_default_domain() is losing the iommu_get_def_domain_type() 
vs. iommu_def_domain_type distinction, but in the former case a driver's 
.def_domain_type should definitely not be returning a type which that 
driver doesn't support, thus allocation failure would only really 
represent unexpected OOM conditions, where the retry would likely fail 
as well, and the whole system is probably dying anyway. If we're 
refactoring the whole lot, though, we may as well make it logically 
consistent.

Thanks,
Robin.

>> However, this does not apply to use cases that change the default domain
>> through sysfs. In such cases, it seems that we should directly return
>> failure (-ENODEV) and tell the user that the iommu driver does not
>> support identity domain.
> 
> And with the fix below it will return ENODEV rather than autoselect DMA.
> 
>>> +	/*
>>> +	 * There are still some drivers which don't support default domains, so
>>> +	 * we ignore the failure and leave group->default_domain NULL.
>>> +	 *
>>> +	 * We assume that the iommu driver starts up the device in
>>> +	 * 'set_platform_dma_ops' mode if it does not support default domains.
>>> +	 */
>>> +	if (!dom) {
>>> +		ret = 0;
>>> +		goto out_set;
>>> +	}
>>
>> Should we call set_platform_dma_ops here?
> 
> It could be done and should be harmless, but the driver is supposed to
> start up in that mode so don't need to explicitly enter it when
> plugging the device.. I kept things as-is
> 
>> The existing default domain
>> (if exists) will be freed below. But the iommu driver doesn't know about
>> this. It probably will create a UAF case?
> 
> This is a bug:
> 
> 	if (!dom) {
> 		/* Once in default_domain mode we never leave */
> 		if (group->default_domain)
> 			return -ENODEV;
> 
> So default_domain is either NULL forever or set to something forever.
> 
> Jason

Re: [PATCH v2 12/14] iommu: Consolidate the default_domain setup to one function

[Jason Gunthorpe]

On Thu, Mar 30, 2023 at 07:23:56PM +0100, Robin Murphy wrote:

> > I changed it like this:
> > 
> > 	dom = __iommu_domain_alloc(bus, req_type);
> > 	if (!dom && !target_type &&
> > 	    (req_type == IOMMU_DOMAIN_IDENTITY ||
> > 	     req_type == IOMMU_DOMAIN_DMA_FQ)) {
> > 		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
> > 
> > So the auto selection only happens if the target_type is not automatic.
> 
> You mean "is automatic"?

Yep

> I'm missing how target_type and req_type are
> related here, but the general principle should be that if we have derived
> the type from iommu_def_domain_type, because we had nothing more specific
> from either a sysfs request or iommu_get_def_domain_type(), then a
> !IOMMU_DOMAIN_DMA failure can retry with IOMMU_DOMAIN_DMA. Otherwise any
> failure should be final.

Ok, can do

Jason

[PATCH v2 13/14] iommu: Remove __iommu_group_for_each_dev()

[Jason Gunthorpe]

The last two users of it are quite trivial, just open code the one line
loop.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 53 ++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 26129423fa7b95..71c8198f916715 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1160,20 +1160,6 @@ void iommu_group_remove_device(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_group_remove_device);
 
-static int __iommu_group_for_each_dev(struct iommu_group *group, void *data,
-				      int (*fn)(struct device *, void *))
-{
-	struct group_device *device;
-	int ret = 0;
-
-	for_each_group_device(group, device) {
-		ret = fn(device->dev, data);
-		if (ret)
-			break;
-	}
-	return ret;
-}
-
 /**
  * iommu_group_for_each_dev - iterate over each device in the group
  * @group: the group
@@ -1188,10 +1174,15 @@ static int __iommu_group_for_each_dev(struct iommu_group *group, void *data,
 int iommu_group_for_each_dev(struct iommu_group *group, void *data,
 			     int (*fn)(struct device *, void *))
 {
-	int ret;
+	struct group_device *device;
+	int ret = 0;
 
 	mutex_lock(&group->mutex);
-	ret = __iommu_group_for_each_dev(group, data, fn);
+	for_each_group_device(group, device) {
+		ret = fn(device->dev, data);
+		if (ret)
+			break;
+	}
 	mutex_unlock(&group->mutex);
 
 	return ret;
@@ -1879,20 +1870,12 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 	return ret;
 }
 
-static int iommu_group_do_probe_finalize(struct device *dev, void *data)
+static void iommu_group_do_probe_finalize(struct device *dev)
 {
 	const struct iommu_ops *ops = dev_iommu_ops(dev);
 
 	if (ops->probe_finalize)
 		ops->probe_finalize(dev);
-
-	return 0;
-}
-
-static void __iommu_group_dma_finalize(struct iommu_group *group)
-{
-	__iommu_group_for_each_dev(group, group->default_domain,
-				   iommu_group_do_probe_finalize);
 }
 
 int bus_iommu_probe(struct bus_type *bus)
@@ -1911,6 +1894,8 @@ int bus_iommu_probe(struct bus_type *bus)
 		return ret;
 
 	list_for_each_entry_safe(group, next, &group_list, entry) {
+		struct group_device *gdev;
+
 		mutex_lock(&group->mutex);
 
 		/* Remove item from the list */
@@ -1924,7 +1909,15 @@ int bus_iommu_probe(struct bus_type *bus)
 			}
 		}
 		mutex_unlock(&group->mutex);
-		__iommu_group_dma_finalize(group);
+
+		/*
+		 * Mis-locked because the ops->probe_finalize() call-back of
+		 * some IOMMU drivers calls arm_iommu_attach_device() which
+		 * in-turn might call back into IOMMU core code, where it tries
+		 * to take group->mutex, resulting in a deadlock.
+		 */
+		for_each_group_device(group, gdev)
+			iommu_group_do_probe_finalize(gdev->dev);
 	}
 
 	return 0;
@@ -2969,8 +2962,12 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 	mutex_unlock(&group->mutex);
 
 	/* Make sure dma_ops is appropriatley set */
-	if (!ret)
-		__iommu_group_dma_finalize(group);
+	if (!ret) {
+		struct group_device *gdev;
+
+		for_each_group_device(group, gdev)
+			iommu_group_do_probe_finalize(gdev->dev);
+	}
 
 	return ret ?: count;
 }
-- 
2.40.0

Re: [PATCH v2 13/14] iommu: Remove __iommu_group_for_each_dev()

[Baolu Lu]

On 2023/3/30 7:40, Jason Gunthorpe wrote:
> The last two users of it are quite trivial, just open code the one line
> loop.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

[PATCH v2 14/14] iommu: Tidy the control flow in iommu_group_store_type()

[Jason Gunthorpe]

Use a normal "goto unwind" instead of trying to be clever with checking
!ret and manually managing the unlock.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 71c8198f916715..2a63fbdcb76095 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2914,6 +2914,7 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
 static ssize_t iommu_group_store_type(struct iommu_group *group,
 				      const char *buf, size_t count)
 {
+	struct group_device *gdev;
 	int ret, req_type;
 
 	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
@@ -2938,20 +2939,23 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 	if (req_type == IOMMU_DOMAIN_DMA_FQ &&
 	    group->default_domain->type == IOMMU_DOMAIN_DMA) {
 		ret = iommu_dma_init_fq(group->default_domain);
-		if (!ret)
-			group->default_domain->type = IOMMU_DOMAIN_DMA_FQ;
-		mutex_unlock(&group->mutex);
+		if (ret)
+			goto out_unlock;
 
-		return ret ?: count;
+		group->default_domain->type = IOMMU_DOMAIN_DMA_FQ;
+		ret = count;
+		goto out_unlock;
 	}
 
 	/* Otherwise, ensure that device exists and no driver is bound. */
 	if (list_empty(&group->devices) || group->owner_cnt) {
-		mutex_unlock(&group->mutex);
-		return -EPERM;
+		ret = -EPERM;
+		goto out_unlock;
 	}
 
 	ret = iommu_setup_default_domain(group, req_type);
+	if (ret)
+		goto out_unlock;
 
 	/*
 	 * Release the mutex here because ops->probe_finalize() call-back of
@@ -2962,13 +2966,12 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 	mutex_unlock(&group->mutex);
 
 	/* Make sure dma_ops is appropriatley set */
-	if (!ret) {
-		struct group_device *gdev;
-
-		for_each_group_device(group, gdev)
-			iommu_group_do_probe_finalize(gdev->dev);
-	}
+	for_each_group_device(group, gdev)
+		iommu_group_do_probe_finalize(gdev->dev);
+	return count;
 
+out_unlock:
+	mutex_unlock(&group->mutex);
 	return ret ?: count;
 }
 
-- 
2.40.0

Re: [PATCH v2 14/14] iommu: Tidy the control flow in iommu_group_store_type()

[Baolu Lu]

On 2023/3/30 7:40, Jason Gunthorpe wrote:
> Use a normal "goto unwind" instead of trying to be clever with checking
> !ret and manually managing the unlock.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu