[PATCH 1/1] iommu: Hold group references across bus_iommu_probe()

[PATCH 1/1] iommu: Hold group references across bus_iommu_probe()

[Peter Chen]

bus_iommu_probe() collects the groups that are linked through
group->entry, but the list does not hold a reference on them. Only the
member devices do. A device of such a group may be released while the
bus walk is still in progress, for example by a BUS_NOTIFY_REMOVED_DEVICE
for a platform device that is depopulated concurrently. If the last
iommu_group_put() then frees the group while it is still linked on the
list, the second loop of bus_iommu_probe() walks and unlinks freed
memory.

This is the "iommu_bus_notifier() being triggered during
bus_iommu_probe()" case that commit da33e87bd2bf ("iommu: Handle yet
another race around registration") left open. The sequence is like
below:
- Process 'A' creates the platform device 'T'.
- Process 'B' (eg, arm_smmu_device_probe) walks the iommu bus, and
  allocates new group ID 'X' for this device 'T'.
- Process 'A' deletes the platform device 'T', and frees its iommu group.
- Process 'B' finishes walking the iommu bus, and tries to get the iommu
  group from the list, but that iommu group has been freed.

The kernel dump is like below:
  list_del corruption. next->prev should be ffffff8064a984e8,
                       but was 0000000000000000. (next=ffffff8064a9a6e8)
  kernel BUG at lib/list_debug.c:67!
  Call trace:
   __list_del_entry_valid_or_report+0x148/0x14c (P)
   iommu_device_register+0x1d8/0x268
   arm_smmu_device_probe+0x588/0x624 [arm_smmu]
   platform_probe+0x74/0xb8
   ...
   deferred_probe_work_func+0xa4/0xf4

where next points at ->entry (offset 232) of a freed kmalloc-512 object,
the released iommu_group of the depopulated device. That was seen
on an Android 6.18-based kernel, but the code involved is unchanged in
mainline.

Take a group reference when a group is added to the list, and drop it
once bus_iommu_probe() is done with the group. With the group kept
alive, a group that has lost all of its devices in the meantime can
simply be skipped: there is nothing left to attach a default domain to.

Also drain the list on the error paths. Returning early used to leave
the remaining groups linked to the dead on-stack list head, so
list_empty(&group->entry) stays false for them and a later
bus_iommu_probe() would never queue them for default domain setup again.

Fixes: 41df6dcc0a3f ("iommu: Keep a list of allocated groups in __iommu_probe_device()")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Peter Chen <peter.chen@oss.qualcomm.com>
---
 drivers/iommu/iommu.c | 41 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b486b8bbd1fc..6965b89e76bb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -682,10 +682,14 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 		/*
 		 * With a group_list argument we defer the default_domain setup
 		 * to the caller by providing a de-duplicated list of groups
-		 * that need further setup.
+		 * that need further setup. The list holds a reference on each
+		 * group, as its devices may be released before the caller
+		 * gets to it.
 		 */
-		if (list_empty(&group->entry))
+		if (list_empty(&group->entry)) {
+			iommu_group_ref_get(group);
 			list_add_tail(&group->entry, group_list);
+		}
 	}
 
 	if (group->default_domain)
@@ -1951,6 +1955,18 @@ static void iommu_group_do_probe_finalize(struct device *dev)
 		ops->probe_finalize(dev);
 }
 
+static void iommu_group_list_put(struct list_head *group_list)
+{
+	struct iommu_group *group, *next;
+
+	list_for_each_entry_safe(group, next, group_list, entry) {
+		mutex_lock(&group->mutex);
+		list_del_init(&group->entry);
+		mutex_unlock(&group->mutex);
+		iommu_group_put(group);
+	}
+}
+
 static int bus_iommu_probe(const struct bus_type *bus)
 {
 	struct iommu_group *group, *next;
@@ -1959,7 +1975,7 @@ static int bus_iommu_probe(const struct bus_type *bus)
 
 	ret = bus_for_each_dev(bus, NULL, &group_list, probe_iommu_group);
 	if (ret)
-		return ret;
+		goto err_put_groups;
 
 	list_for_each_entry_safe(group, next, &group_list, entry) {
 		struct group_device *gdev;
@@ -1969,6 +1985,16 @@ static int bus_iommu_probe(const struct bus_type *bus)
 		/* Remove item from the list */
 		list_del_init(&group->entry);
 
+		/*
+		 * The bus notifier may have released every device of the group
+		 * since it was added to the list; there is nothing to set up.
+		 */
+		if (list_empty(&group->devices)) {
+			mutex_unlock(&group->mutex);
+			iommu_group_put(group);
+			continue;
+		}
+
 		/*
 		 * We go to the trouble of deferred default domain creation so
 		 * that the cross-group default domain type and the setup of the
@@ -1977,7 +2003,8 @@ static int bus_iommu_probe(const struct bus_type *bus)
 		ret = iommu_setup_default_domain(group, 0);
 		if (ret) {
 			mutex_unlock(&group->mutex);
-			return ret;
+			iommu_group_put(group);
+			goto err_put_groups;
 		}
 		for_each_group_device(group, gdev)
 			iommu_setup_dma_ops(gdev->dev, group->default_domain);
@@ -1991,9 +2018,15 @@ static int bus_iommu_probe(const struct bus_type *bus)
 		 */
 		for_each_group_device(group, gdev)
 			iommu_group_do_probe_finalize(gdev->dev);
+
+		iommu_group_put(group);
 	}
 
 	return 0;
+
+err_put_groups:
+	iommu_group_list_put(&group_list);
+	return ret;
 }
 
 /**
-- 
2.43.0