[RFC PATCH 0/3] iommu: Reduce iommu_probe_device_lock contention

[RFC PATCH 0/3] iommu: Reduce iommu_probe_device_lock contention

[Pavol Sakac]

iommu_probe_device_lock is a file-scope mutex held across
__iommu_probe_device(): every device's IOMMU probe serializes against
every other. With "PCI/IOV: Initialize virtual functions in
parallel" [1] fanning an SR-IOV enable across CPUs, it collects 94.7%
of all lock wait. Patch 1 gathers per-member sysfs publication into one
all-or-nothing helper (no functional change); patches 2-3 move that
publication and first-device default-domain setup into per-group
finalisation after the global unlock, under group->mutex, mirroring the
removal path and bus_iommu_probe().

Lock statistics and SR-IOV init time for 4x PF (NVMe, 255 VFs each), on
the reproducer from the parallel VF initialization cover letter [1]:

  lock_stat:
  Lock                     wait: Before     After   contentions: Before   After
  iommu_probe_device_lock      25507 ms  10471 ms                  1143     841
  &root->kernfs_rwsem            942 ms   1834 ms                 93208  116316
  &vfio.group_lock               425 ms   3614 ms                   497     736

  avg wait per acquisition  6.3 ms -> 2.6 ms (4080 acq., both arms)

  Stage                 SR-IOV init time:
  S0 (baseline)         3027 ms
  S1                     999 ms
  S2 (this series)       995 ms

Reproducer disclaimer:
I lean primarily on lock_stat numbers to defend the improvements. In
the reproducer, this lock's residual hold dominates the window and
masks the later series' wall-time gains, more in [1].

This is relief, not removal: the lock still has the highest wait
time in the profile after this series. Wait per acquisition drops
from 6.3 ms to 2.6 ms, which is what makes the smaller
serialization points behind it measurable for the later series.

RFC on the direction: The comment in __iommu_probe_device() expects
the lock to narrow to device_lock() once the ACPI/OF replay calls
are cleaned up. I could not make that work for the whole section:
group formation in ops->device_group() is a cross-device decision a
per-device lock cannot order. This series instead moves the work
that needs no global ordering out of the section. Is that an
acceptable step, or is there a scoping or removal plan this should
wait for?

A second question: I have measured where 90% of the residual hold goes:
get_pci_alias_group() walks every PCI device in the system to find
same-bus DMA aliases. It runs once per device probed, so once per VF,
under this lock, and the VFs keep growing the list it walks. A prototype
that skips the walk when no device has a dma_alias_mask and this device
has no pci_real_dma_dev() override cuts this lock's hold time by about
90%, for the same acquisitions and the same groups. I am not proposing
it here, as I do not have the time to get it right this cycle. How can
we optimize this preferably in O(1) time?

The lock_stat and timing figures come from the public reproducer. The
full series has also been tested on current datacenter server hardware
with thousands of VFs.

[1] https://lore.kernel.org/r/20260911-vfopt-s1-v1-0-693271dc0226@amazon.de

Pavol Sakac (3):
  iommu: split sysfs link publication out of iommu_group_alloc_device()
  iommu: create device sysfs links outside iommu_probe_device_lock
  iommu: set up the default domain outside iommu_probe_device_lock

 drivers/iommu/iommu.c | 197 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 160 insertions(+), 37 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.47.3

[RFC PATCH 1/3] iommu: split sysfs link publication out of iommu_group_alloc_device()

[Pavol Sakac]

iommu_group_alloc_device() both allocates a group_device and publishes
it in sysfs (the "iommu_group" link and the group "devices/" member
link with its .%d rename loop), while the iommu instance links are
published separately in iommu_init_device() and removed in
iommu_deinit_device().

Gather all per-member publication into one helper,
iommu_group_link_device(), run under group->mutex, and record success
in a new group_device::linked flag. Teardown moves next to the member
link removal in __iommu_group_free_device(), which consults the flag so
an unpublished member skips sysfs.

No functional change intended; this gives publication a single seam so
a later commit can move it out of iommu_probe_device_lock.

Assisted-by: LLM
Signed-off-by: Pavol Sakac <sakacpav@amazon.de>
---
 drivers/iommu/iommu.c | 91 +++++++++++++++++++++++++++++++------------
 1 file changed, 67 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index cd1bca7ede9a..5f92981d9f34 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -77,6 +77,8 @@ struct group_device {
 	struct list_head list;
 	struct device *dev;
 	char *name;
+	/* Membership published in sysfs; set under group->mutex */
+	bool linked;
 	/*
 	 * Device is blocked for a pending recovery while its group->domain is
 	 * retained. This can happen when:
@@ -166,6 +168,8 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 				      const char *buf, size_t count);
 static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
 						     struct device *dev);
+static int iommu_group_link_device(struct iommu_group *group,
+				   struct group_device *device);
 static void __iommu_group_free_device(struct iommu_group *group,
 				      struct group_device *grp_dev);
 static void iommu_domain_init(struct iommu_domain *domain, unsigned int type,
@@ -515,16 +519,12 @@ static int iommu_init_device(struct device *dev)
 	}
 	dev->iommu->iommu_dev = iommu_dev;
 
-	ret = iommu_device_link(iommu_dev, dev);
-	if (ret)
-		goto err_release;
-
 	group = ops->device_group(dev);
 	if (WARN_ON_ONCE(group == NULL))
 		group = ERR_PTR(-EINVAL);
 	if (IS_ERR(group)) {
 		ret = PTR_ERR(group);
-		goto err_unlink;
+		goto err_release;
 	}
 	dev->iommu_group = group;
 
@@ -533,8 +533,6 @@ static int iommu_init_device(struct device *dev)
 		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
 	return 0;
 
-err_unlink:
-	iommu_device_unlink(iommu_dev, dev);
 err_release:
 	if (ops->release_device)
 		ops->release_device(dev);
@@ -553,8 +551,6 @@ static void iommu_deinit_device(struct device *dev)
 
 	lockdep_assert_held(&group->mutex);
 
-	iommu_device_unlink(dev->iommu->iommu_dev, dev);
-
 	/*
 	 * release_device() must stop using any attached domain on the device.
 	 * If there are still other devices in the group, they are not affected
@@ -667,6 +663,11 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	 */
 	list_add_tail(&gdev->list, &group->devices);
 	WARN_ON(group->default_domain && !group->domain);
+
+	ret = iommu_group_link_device(group, gdev);
+	if (ret)
+		goto err_remove_gdev;
+
 	if (group->default_domain)
 		iommu_create_device_direct_mappings(group->default_domain, dev);
 	if (group->domain) {
@@ -729,10 +730,14 @@ static void __iommu_group_free_device(struct iommu_group *group,
 {
 	struct device *dev = grp_dev->dev;
 
-	sysfs_remove_link(group->devices_kobj, grp_dev->name);
-	sysfs_remove_link(&dev->kobj, "iommu_group");
+	if (grp_dev->linked) {
+		if (dev_has_iommu(dev))
+			iommu_device_unlink(dev->iommu->iommu_dev, dev);
+		sysfs_remove_link(group->devices_kobj, grp_dev->name);
+		sysfs_remove_link(&dev->kobj, "iommu_group");
 
-	trace_remove_device_from_group(group->id, dev);
+		trace_remove_device_from_group(group->id, dev);
+	}
 
 	/*
 	 * If the group has become empty then ownership must have been
@@ -1266,7 +1271,6 @@ static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
 static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
 						     struct device *dev)
 {
-	int ret, i = 0;
 	struct group_device *device;
 
 	device = kzalloc_obj(*device);
@@ -1275,11 +1279,40 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
 
 	device->dev = dev;
 
+	device->name = kasprintf(GFP_KERNEL, "%s", kobject_name(&dev->kobj));
+	if (!device->name) {
+		kfree(device);
+		dev_err(dev, "Failed to add to iommu group %d: %d\n",
+			group->id, -ENOMEM);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	return device;
+}
+
+/*
+ * Publish a member's sysfs links under group->mutex.  All-or-nothing:
+ * on failure nothing is left behind and gdev->linked stays false, so
+ * __iommu_group_free_device() removes only what was published.
+ */
+static int iommu_group_link_device(struct iommu_group *group,
+				   struct group_device *device)
+{
+	struct device *dev = device->dev;
+	int ret, i = 0;
+
+	lockdep_assert_held(&group->mutex);
+
+	if (dev_has_iommu(dev)) {
+		ret = iommu_device_link(dev->iommu->iommu_dev, dev);
+		if (ret)
+			goto err_out;
+	}
+
 	ret = sysfs_create_link(&dev->kobj, &group->kobj, "iommu_group");
 	if (ret)
-		goto err_free_device;
+		goto err_unlink_iommu;
 
-	device->name = kasprintf(GFP_KERNEL, "%s", kobject_name(&dev->kobj));
 rename:
 	if (!device->name) {
 		ret = -ENOMEM;
@@ -1299,23 +1332,25 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
 						 kobject_name(&dev->kobj), i++);
 			goto rename;
 		}
-		goto err_free_name;
+		goto err_remove_link;
 	}
 
+	device->linked = true;
+
 	trace_add_device_to_group(group->id, dev);
 
 	dev_info(dev, "Adding to iommu group %d\n", group->id);
 
-	return device;
+	return 0;
 
-err_free_name:
-	kfree(device->name);
 err_remove_link:
 	sysfs_remove_link(&dev->kobj, "iommu_group");
-err_free_device:
-	kfree(device);
+err_unlink_iommu:
+	if (dev_has_iommu(dev))
+		iommu_device_unlink(dev->iommu->iommu_dev, dev);
+err_out:
 	dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id, ret);
-	return ERR_PTR(ret);
+	return ret;
 }
 
 /**
@@ -1329,15 +1364,23 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
 int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 {
 	struct group_device *gdev;
+	int ret;
 
 	gdev = iommu_group_alloc_device(group, dev);
 	if (IS_ERR(gdev))
 		return PTR_ERR(gdev);
 
+	mutex_lock(&group->mutex);
+	ret = iommu_group_link_device(group, gdev);
+	if (ret) {
+		mutex_unlock(&group->mutex);
+		kfree(gdev->name);
+		kfree(gdev);
+		return ret;
+	}
+
 	iommu_group_ref_get(group);
 	dev->iommu_group = group;
-
-	mutex_lock(&group->mutex);
 	list_add_tail(&gdev->list, &group->devices);
 	mutex_unlock(&group->mutex);
 	return 0;
-- 
2.47.3

[RFC PATCH 2/3] iommu: create device sysfs links outside iommu_probe_device_lock

[Pavol Sakac]

Parallel device probes convoy on iommu_probe_device_lock, and parallel VF
initialisation shows it as a top contention source, with per-member sysfs
publication the dominant term under the hold. Publication needs no global
ordering: removal already does the reverse under group->mutex alone in
__iommu_group_free_device().

Pin the group under the global lock, drop it, and publish every member
still lacking links under group->mutex. A device joining a group that
already has a domain is published under the global lock instead, because
no later pass is guaranteed to visit it; members created by
probe_iommu_group() are published by bus_iommu_probe()'s existing drain.

The group reference is needed because not every caller pins the device:
iommu_add_device() on powerpc, the __init sweep in
probe_acpi_namespace_devices() and the of_dma_configure() replay run
neither inside device_add() nor under device_lock().

A sibling whose publication failed stays unpublished, retried by any later
finalisation or drain; in the worst case it waits until a new member joins,
the same terminal shape a failed bus_iommu_probe() drain leaves today. On
the hotplug path the links exist before device_add() emits KOBJ_ADD, so
udev cannot tell the difference.

A failed call also removes the probing device itself from the group:
iommu_probe_device() early-exits on membership, so a member left behind
would turn a retried probe into a no-op success while the group's members
remain unpublished.

Assisted-by: LLM
Signed-off-by: Pavol Sakac <sakacpav@amazon.de>
---
 drivers/iommu/iommu.c | 70 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 65 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 5f92981d9f34..a5e3327aeaca 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -663,11 +663,12 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	 */
 	list_add_tail(&gdev->list, &group->devices);
 	WARN_ON(group->default_domain && !group->domain);
-
-	ret = iommu_group_link_device(group, gdev);
-	if (ret)
-		goto err_remove_gdev;
-
+	if (group->domain || group->default_domain) {
+		/* No later pass is guaranteed to publish this member. */
+		ret = iommu_group_link_device(group, gdev);
+		if (ret)
+			goto err_remove_gdev;
+	}
 	if (group->default_domain)
 		iommu_create_device_direct_mappings(group->default_domain, dev);
 	if (group->domain) {
@@ -710,19 +711,68 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 int iommu_probe_device(struct device *dev)
 {
 	const struct iommu_ops *ops;
+	struct group_device *gdev, *own;
+	struct iommu_group *group;
 	int ret;
 
 	mutex_lock(&iommu_probe_device_lock);
+	/* Already probed; publication belongs to the creating call. */
+	if (dev->iommu_group) {
+		mutex_unlock(&iommu_probe_device_lock);
+		goto probe_finalize;
+	}
 	ret = __iommu_probe_device(dev, NULL);
+	if (!ret) {
+		/*
+		 * Not every caller pins the device, so pin the group across
+		 * the unlock.
+		 */
+		group = dev->iommu_group;
+		iommu_group_ref_get(group);
+	}
 	mutex_unlock(&iommu_probe_device_lock);
 	if (ret)
 		return ret;
 
+	mutex_lock(&group->mutex);
+	/* iommu_group_link_device() is all-or-nothing. */
+	for_each_group_device(group, gdev) {
+		if (gdev->linked)
+			continue;
+		ret = iommu_group_link_device(group, gdev);
+		if (ret)
+			goto err_remove_device;
+	}
+	mutex_unlock(&group->mutex);
+	iommu_group_put(group);
+
+probe_finalize:
 	ops = dev_iommu_ops(dev);
 	if (ops->probe_finalize)
 		ops->probe_finalize(dev);
 
 	return 0;
+
+err_remove_device:
+	/* Retried probes early-exit on membership; failure must undo it. */
+	own = NULL;
+	for_each_group_device(group, gdev) {
+		if (gdev->dev == dev) {
+			own = gdev;
+			break;
+		}
+	}
+	if (own) {
+		list_del(&own->list);
+		__iommu_group_free_device(group, own);
+		iommu_deinit_device(dev);
+	}
+	mutex_unlock(&group->mutex);
+	if (own)
+		iommu_group_put(group);	/* iommu_init_device()'s reference */
+	iommu_group_put(group);		/* the reference taken above */
+
+	return ret;
 }
 
 static void __iommu_group_free_device(struct iommu_group *group,
@@ -2012,6 +2062,16 @@ static int bus_iommu_probe(const struct bus_type *bus)
 		/* Remove item from the list */
 		list_del_init(&group->entry);
 
+		for_each_group_device(group, gdev) {
+			if (gdev->linked)
+				continue;
+			ret = iommu_group_link_device(group, gdev);
+			if (ret) {
+				mutex_unlock(&group->mutex);
+				return ret;
+			}
+		}
+
 		/*
 		 * We go to the trouble of deferred default domain creation so
 		 * that the cross-group default domain type and the setup of the
-- 
2.47.3

[RFC PATCH 3/3] iommu: set up the default domain outside iommu_probe_device_lock

[Pavol Sakac]

Default domain allocation, attach and direct-mapping setup for a group's
first device still run inline under iommu_probe_device_lock, though they
need only group->mutex: iommu_group_store_type() does this at runtime
under group->mutex alone, and bus_iommu_probe() already defers it via the
group_list. With publication moved out it is the largest term left under
the hold, and parallel VF initialisation convoys behind it.

Use the group_list deferral for the singleton probe path too:
iommu_probe_device() passes a list and runs the setup after the global
unlock, under group->mutex. The global lock's original purpose - commit
01657bc14a39 ("iommu: Avoid races around device probe") - kept two devices
of one group from double-initialising it; that invariant moves to
group->mutex, where the setup is rechecked: of two racing siblings the
first to find the group unset sets it up for every member, and the rest
skip.

A failed setup unwinds only the calling device, like a failed publication;
siblings wait in -EPROBE_DEFER until a later probe finalises the group, or
until a new member joins if the failing call had claimed a bus-scanned
group's queued entry - the same terminal shape a failed bus_iommu_probe()
drain leaves today. A domain the failed setup published stays attached
(the first attach is IOMMU_SET_DOMAIN_MUST_SUCCEED), so members left
behind get the DMA ops it owed them.

bus_iommu_probe() rechecks group->default_domain, as a hotplug probe can
finalise a queued group before the drain takes the mutex.

Assisted-by: LLM
Signed-off-by: Pavol Sakac <sakacpav@amazon.de>
---
 drivers/iommu/iommu.c | 48 ++++++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index a5e3327aeaca..48fe22bbd1bc 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -657,10 +657,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 		goto err_put_group;
 	}
 
-	/*
-	 * The gdev must be in the list before calling
-	 * iommu_setup_default_domain()
-	 */
+	/* List the gdev first: the finaliser covers every listed member. */
 	list_add_tail(&gdev->list, &group->devices);
 	WARN_ON(group->default_domain && !group->domain);
 	if (group->domain || group->default_domain) {
@@ -676,15 +673,10 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 						0);
 		if (ret)
 			goto err_remove_gdev;
-	} else if (!group->default_domain && !group_list) {
-		ret = iommu_setup_default_domain(group, 0);
-		if (ret)
-			goto err_remove_gdev;
 	} else if (!group->default_domain) {
 		/*
-		 * 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.
+		 * Defer setup to the caller draining group_list; both in-tree
+		 * callers pass one.
 		 */
 		if (list_empty(&group->entry))
 			list_add_tail(&group->entry, group_list);
@@ -713,6 +705,7 @@ int iommu_probe_device(struct device *dev)
 	const struct iommu_ops *ops;
 	struct group_device *gdev, *own;
 	struct iommu_group *group;
+	LIST_HEAD(group_list);
 	int ret;
 
 	mutex_lock(&iommu_probe_device_lock);
@@ -721,7 +714,7 @@ int iommu_probe_device(struct device *dev)
 		mutex_unlock(&iommu_probe_device_lock);
 		goto probe_finalize;
 	}
-	ret = __iommu_probe_device(dev, NULL);
+	ret = __iommu_probe_device(dev, &group_list);
 	if (!ret) {
 		/*
 		 * Not every caller pins the device, so pin the group across
@@ -735,6 +728,9 @@ int iommu_probe_device(struct device *dev)
 		return ret;
 
 	mutex_lock(&group->mutex);
+	/* Only the caller that queued the entry may unlink it. */
+	if (!list_empty(&group_list))
+		list_del_init(&group->entry);
 	/* iommu_group_link_device() is all-or-nothing. */
 	for_each_group_device(group, gdev) {
 		if (gdev->linked)
@@ -743,6 +739,20 @@ int iommu_probe_device(struct device *dev)
 		if (ret)
 			goto err_remove_device;
 	}
+	/*
+	 * First thread to find the group unset sets it up for every member;
+	 * binding needs a default domain, so no DMA races the window.
+	 */
+	if (!list_empty(&group->devices) &&
+	    !group->domain && !group->default_domain) {
+		ret = iommu_setup_default_domain(group, 0);
+		if (ret)
+			goto err_remove_device;
+		for_each_group_device(group, gdev)
+			if (dev_has_iommu(gdev->dev))
+				iommu_setup_dma_ops(gdev->dev,
+						    group->default_domain);
+	}
 	mutex_unlock(&group->mutex);
 	iommu_group_put(group);
 
@@ -765,8 +775,15 @@ int iommu_probe_device(struct device *dev)
 	if (own) {
 		list_del(&own->list);
 		__iommu_group_free_device(group, own);
-		iommu_deinit_device(dev);
 	}
+	/* Members left behind stay attached; give them the DMA ops owed. */
+	if (group->default_domain)
+		for_each_group_device(group, gdev)
+			if (dev_has_iommu(gdev->dev))
+				iommu_setup_dma_ops(gdev->dev,
+						    group->default_domain);
+	if (own)
+		iommu_deinit_device(dev);
 	mutex_unlock(&group->mutex);
 	if (own)
 		iommu_group_put(group);	/* iommu_init_device()'s reference */
@@ -2077,7 +2094,10 @@ static int bus_iommu_probe(const struct bus_type *bus)
 		 * that the cross-group default domain type and the setup of the
 		 * IOMMU_RESV_DIRECT will work correctly in non-hotpug scenarios.
 		 */
-		ret = iommu_setup_default_domain(group, 0);
+		ret = 0;
+		/* A hotplug probe may have finalised this group meanwhile. */
+		if (!group->default_domain)
+			ret = iommu_setup_default_domain(group, 0);
 		if (ret) {
 			mutex_unlock(&group->mutex);
 			return ret;
-- 
2.47.3

Re: [RFC PATCH 2/3] iommu: create device sysfs links outside iommu_probe_device_lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Early success return on `dev->iommu_group` allows concurrent probes to bypass synchronization, leading to Use-After-Free if publication fails.
--

commit 4fb83220aac8b0e212834d62f2d81e2c473af4b4
Author: Pavol Sakac <sakacpav@amazon.de>

iommu: create device sysfs links outside iommu_probe_device_lock

This patch optimizes parallel device probes by moving sysfs link publication
out of the iommu_probe_device_lock. It introduces a mechanism where devices
are added to the group first, and later passes attempt to establish the
sysfs links outside the lock.

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 5f92981d9f345..a5e3327aeacaf 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -710,19 +711,68 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>  int iommu_probe_device(struct device *dev)
>  {
>  	const struct iommu_ops *ops;
> +	struct group_device *gdev, *own;
> +	struct iommu_group *group;
>  	int ret;
>  
>  	mutex_lock(&iommu_probe_device_lock);
> +	/* Already probed; publication belongs to the creating call. */
> +	if (dev->iommu_group) {
> +		mutex_unlock(&iommu_probe_device_lock);
> +		goto probe_finalize;
> +	}

[Severity: High]
Could this early return allow a concurrent thread to bypass synchronization
and proceed to finalize the device while the primary thread is still
executing publication steps?

If two threads concurrently call iommu_probe_device() for the same device
without device_lock() held, Thread B could see dev->iommu_group already
set by Thread A and jump straight to probe_finalize.

[ ... ]

> +probe_finalize:
>  	ops = dev_iommu_ops(dev);
>  	if (ops->probe_finalize)
>  		ops->probe_finalize(dev);

[Severity: High]
If Thread B jumps to probe_finalize, could it dereference dev->iommu
via dev_iommu_ops(dev) while Thread A executes the err_remove_device path?

[ ... ]

> +err_remove_device:
> +	/* Retried probes early-exit on membership; failure must undo it. */
> +	own = NULL;
> +	for_each_group_device(group, gdev) {
> +		if (gdev->dev == dev) {
> +			own = gdev;
> +			break;
> +		}
> +	}
> +	if (own) {
> +		list_del(&own->list);
> +		__iommu_group_free_device(group, own);
> +		iommu_deinit_device(dev);
> +	}

[Severity: High]
Does calling iommu_deinit_device(dev) free dev->iommu here in Thread A,
leading to a use-after-free when Thread B accesses it in probe_finalize?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-vfopt-s2-v1-0-fff3db7e01c2@amazon.de?part=2

Re: [RFC PATCH 1/3] iommu: split sysfs link publication out of iommu_group_alloc_device()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Dangling stack pointer in `struct iommu_group` when `bus_iommu_probe` fails.
--

commit 40e2c611e51a4bd6297705c58373104232dd8541
Author: Pavol Sakac <sakacpav@amazon.de>
Subject: iommu: split sysfs link publication out of iommu_group_alloc_device()

The patch separates sysfs link publication from iommu_group_alloc_device().
It moves all per-member sysfs publication into a new helper,
iommu_group_link_device(), which sets a linked flag to handle teardown
correctly. This change prepares for a later commit by providing a single
seam for sysfs publication.

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index cd1bca7ede9af..5f92981d9f345 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
[ ... ]
> @@ -667,6 +663,11 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>  	 */
>  	list_add_tail(&gdev->list, &group->devices);
>  	WARN_ON(group->default_domain && !group->domain);
> +
> +	ret = iommu_group_link_device(group, gdev);
> +	if (ret)
> +		goto err_remove_gdev;
> +
>  	if (group->default_domain)
>  		iommu_create_device_direct_mappings(group->default_domain, dev);
>  	if (group->domain) {

[Severity: High]
This is a pre-existing issue, but does returning an error from
__iommu_probe_device() (either here or later in the function) lead to a
dangling stack pointer in the group_list?

Looking at the later part of __iommu_probe_device() in drivers/iommu/iommu.c,
it adds the group to the stack-allocated group_list:

	} else if (!group->default_domain) {
		/*
		 * Defer setup to the caller draining group_list; both in-tree
		 * callers pass one.
		 */
		if (list_empty(&group->entry))
			list_add_tail(&group->entry, group_list);
	}

If a subsequent call to __iommu_probe_device() (or any probe failure during
bus_for_each_dev()) returns an error, bus_iommu_probe() will return immediately:

drivers/iommu/iommu.c:bus_iommu_probe() {
	...
	LIST_HEAD(group_list);
	int ret;

	ret = bus_for_each_dev(bus, NULL, &group_list, probe_iommu_group);
	if (ret)
		return ret;
	...
}

Does this leave long-lived struct iommu_group objects with their group->entry
pointers pointing to the discarded group_list stack frame? This could corrupt
the internal state of the iommu_group and permanently skip deferred setup for
these groups in future retries, because list_empty(&group->entry) will
incorrectly return false.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-vfopt-s2-v1-0-fff3db7e01c2@amazon.de?part=1

Re: [RFC PATCH 3/3] iommu: set up the default domain outside iommu_probe_device_lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] A sysfs linkage failure for one device causes the innocent initiating device to be removed from the IOMMU group and fail its probe.

Pre-existing issues:
- [High] Concurrent device hot-unplug can trigger a Use-After-Free during `bus_iommu_probe` list traversal.
--

commit bbc77bf7c83c2e939356c6c9d4fe1de6d5066ed7
Author: Pavol Sakac <sakacpav@amazon.de>
Subject: iommu: set up the default domain outside iommu_probe_device_lock

This commit defers the default domain setup in iommu_probe_device() until
after the iommu_probe_device_lock is released. It moves the setup under
group->mutex and uses a group_list for deferred initialization, similar to
bus_iommu_probe(). It also updates error handling and rechecks for default
domain setup to prevent races.

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index a5e3327aeacaf..48fe22bbd1bc2 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
[ ... ]
> @@ -735,6 +728,9 @@ int iommu_probe_device(struct device *dev)
>  		return ret;
>  
>  	mutex_lock(&group->mutex);
> +	/* Only the caller that queued the entry may unlink it. */
> +	if (!list_empty(&group_list))
> +		list_del_init(&group->entry);
>  	/* iommu_group_link_device() is all-or-nothing. */
>  	for_each_group_device(group, gdev) {
>  		if (gdev->linked)
>  			continue;
>  		ret = iommu_group_link_device(group, gdev);
>  		if (ret)
>  			goto err_remove_device;
>  	}

[Severity: High]
Does this loop abort the entire group setup and fail the innocent initiating
device if a different device's sysfs linkage fails?

If another device in the group has a sysfs linking failure, this loop will
abort and jump to err_remove_device.

[ ... ]
> @@ -765,8 +775,15 @@ int iommu_probe_device(struct device *dev)
>  	if (own) {
>  		list_del(&own->list);
>  		__iommu_group_free_device(group, own);
> -		iommu_deinit_device(dev);
>  	}

[Severity: High]
Down here in the error handler, it unlinks and frees the innocent initiating
device (own) instead of handling the broken device that failed to link above.

Can this create a permanent cross-device failure state where one broken device
prevents other devices in the same group from probing successfully?

> +	/* Members left behind stay attached; give them the DMA ops owed. */
> +	if (group->default_domain)
> +		for_each_group_device(group, gdev)
> +			if (dev_has_iommu(gdev->dev))
> +				iommu_setup_dma_ops(gdev->dev,
> +						    group->default_domain);
> +	if (own)
> +		iommu_deinit_device(dev);
>  	mutex_unlock(&group->mutex);
>  	if (own)
>  		iommu_group_put(group);	/* iommu_init_device()'s reference */
[ ... ]
> @@ -2077,7 +2094,10 @@ static int bus_iommu_probe(const struct bus_type *bus)
>  		 * that the cross-group default domain type and the setup of the
>  		 * IOMMU_RESV_DIRECT will work correctly in non-hotpug scenarios.
>  		 */
> -		ret = iommu_setup_default_domain(group, 0);
> +		ret = 0;
> +		/* A hotplug probe may have finalised this group meanwhile. */
> +		if (!group->default_domain)
> +			ret = iommu_setup_default_domain(group, 0);

[Severity: High]
This is a pre-existing issue, but can concurrent device hot-unplug trigger a
use-after-free during the list traversal in bus_iommu_probe()?

Earlier in this function, bus_for_each_dev() populates group_list with
group->entry without incrementing the group's reference count. If the last
device in a group is concurrently hot-unplugged after bus_for_each_dev()
completes but before the group is processed in the list_for_each_entry_safe()
loop above this block, the group's reference count drops to zero and the
group is freed.

The subsequent list_for_each_entry_safe() traversal then reads from the freed
group (e.g., group->entry.next) and attempts to take its mutex.

>  		if (ret) {
>  			mutex_unlock(&group->mutex);
>  			return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-vfopt-s2-v1-0-fff3db7e01c2@amazon.de?part=3

Re: [RFC PATCH 0/3] iommu: Reduce iommu_probe_device_lock contention

[Robin Murphy]

On 11/09/2026 1:58 pm, Pavol Sakac wrote:
> iommu_probe_device_lock is a file-scope mutex held across
> __iommu_probe_device(): every device's IOMMU probe serializes against
> every other. With "PCI/IOV: Initialize virtual functions in
> parallel" [1] fanning an SR-IOV enable across CPUs, it collects 94.7%
> of all lock wait. Patch 1 gathers per-member sysfs publication into one
> all-or-nothing helper (no functional change); patches 2-3 move that
> publication and first-device default-domain setup into per-group
> finalisation after the global unlock, under group->mutex, mirroring the
> removal path and bus_iommu_probe().
> 
> Lock statistics and SR-IOV init time for 4x PF (NVMe, 255 VFs each), on
> the reproducer from the parallel VF initialization cover letter [1]:
> 
>    lock_stat:
>    Lock                     wait: Before     After   contentions: Before   After
>    iommu_probe_device_lock      25507 ms  10471 ms                  1143     841
>    &root->kernfs_rwsem            942 ms   1834 ms                 93208  116316
>    &vfio.group_lock               425 ms   3614 ms                   497     736
> 
>    avg wait per acquisition  6.3 ms -> 2.6 ms (4080 acq., both arms)
> 
>    Stage                 SR-IOV init time:
>    S0 (baseline)         3027 ms
>    S1                     999 ms
>    S2 (this series)       995 ms
> 
> Reproducer disclaimer:
> I lean primarily on lock_stat numbers to defend the improvements. In
> the reproducer, this lock's residual hold dominates the window and
> masks the later series' wall-time gains, more in [1].
> 
> This is relief, not removal: the lock still has the highest wait
> time in the profile after this series. Wait per acquisition drops
> from 6.3 ms to 2.6 ms, which is what makes the smaller
> serialization points behind it measurable for the later series.
> 
> RFC on the direction: The comment in __iommu_probe_device() expects
> the lock to narrow to device_lock() once the ACPI/OF replay calls
> are cleaned up. I could not make that work for the whole section:
> group formation in ops->device_group() is a cross-device decision a
> per-device lock cannot order. This series instead moves the work
> that needs no global ordering out of the section. Is that an
> acceptable step, or is there a scoping or removal plan this should
> wait for?

The point of probe_device_lock is to prevent multiple threads trying to
probe the *same* device concurrently; it protects the per-device state
of dev->iommu and dev->iommu_group until the latter is assigned or the
former is cleaned up (depending on how the probe goes). The replay calls
are mostly gone, but the main reason device_lock() still won't work is
that the same problem exists for driver-model-based IOMMU drivers
themselves, since we don't have a good way to avoid bus_iommu_probe()
deadlocking on IOMMU devices that are in the middle of registering
during their own driver bind (not least the caller itself).

> A second question: I have measured where 90% of the residual hold goes:
> get_pci_alias_group() walks every PCI device in the system to find
> same-bus DMA aliases. It runs once per device probed, so once per VF,
> under this lock, and the VFs keep growing the list it walks. A prototype
> that skips the walk when no device has a dma_alias_mask and this device
> has no pci_real_dma_dev() override cuts this lock's hold time by about
> 90%, for the same acquisitions and the same groups. I am not proposing
> it here, as I do not have the time to get it right this cycle. How can
> we optimize this preferably in O(1) time?

TBH that makes it sound like optimising pci_device_group() is the better
thing to do. We were never really meant to have a global lock here - it
was just an acceptable compromise for simplicity at the time - so I'm
still not keen on adding yet more complexity to the probe flow to work
around it as if global serialisation was necessary when it isn't.

Heck, even if you do just want a quick bodge to ease contention then I'd
still lean more towards something more self-contained like this
hometime-on-a-Friday fun I couldn't resist sketching out...

Thanks,
Robin.

----->8-----

From: Robin Murphy <robin.murphy@arm.com>
Subject: [PATCH] UNTESTED: iommu: Reduce iommu_probe_device_lock contention

The purpose of iommu_probe_device_lock was to prevent multiple threads
trying to probe the same device concurrently, it's only global for the
sake of simplicity, as there are still reasons why we can't use
device_lock(), and adding a whole other lock to struct device itself
just for this would be unreasonable.

However, we're now getting sufficiently large systems with enough
devices to start seeing significant contention on this lock, so let's
scale it to a lock table to reduce contention between unrelated devices.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
  drivers/acpi/scan.c      |  6 +++---
  drivers/iommu/iommu.c    | 37 +++++++++++++++++++++++++------------
  drivers/iommu/of_iommu.c |  6 +++---
  include/linux/iommu.h    |  2 +-
  4 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index f48715ed827c..33d6a758042a 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1620,10 +1620,10 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
  	int err;
  
  	/* Serialise to make dev->iommu stable under our potential fwspec */
-	mutex_lock(&iommu_probe_device_lock);
+	mutex_lock(iommu_probe_device_lock(dev));
  	/* If we already translated the fwspec there is nothing left to do */
  	if (dev_iommu_fwspec_get(dev)) {
-		mutex_unlock(&iommu_probe_device_lock);
+		mutex_unlock(iommu_probe_device_lock(dev));
  		return 0;
  	}
  
@@ -1633,7 +1633,7 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
  	if (err && err != -EPROBE_DEFER)
  		err = viot_iommu_configure(dev);
  
-	mutex_unlock(&iommu_probe_device_lock);
+	mutex_unlock(iommu_probe_device_lock(dev));
  
  	return err;
  }
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index cd1bca7ede9a..11519e195aaf 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -381,9 +381,9 @@ int iommu_mock_device_add(struct device *dev, struct iommu_device *iommu)
  {
  	int rc;
  
-	mutex_lock(&iommu_probe_device_lock);
+	mutex_lock(iommu_probe_device_lock(dev));
  	rc = iommu_fwspec_init(dev, iommu->fwnode);
-	mutex_unlock(&iommu_probe_device_lock);
+	mutex_unlock(iommu_probe_device_lock(dev));
  
  	if (rc)
  		return rc;
@@ -400,7 +400,7 @@ static struct dev_iommu *dev_iommu_get(struct device *dev)
  {
  	struct dev_iommu *param = dev->iommu;
  
-	lockdep_assert_held(&iommu_probe_device_lock);
+	lockdep_assert_held(iommu_probe_device_lock(dev));
  
  	if (param)
  		return param;
@@ -457,7 +457,7 @@ void dev_iommu_priv_set(struct device *dev, void *priv)
  {
  	/* FSL_PAMU does something weird */
  	if (!IS_ENABLED(CONFIG_FSL_PAMU))
-		lockdep_assert_held(&iommu_probe_device_lock);
+		lockdep_assert_held(iommu_probe_device_lock(dev));
  	dev->iommu->priv = priv;
  }
  EXPORT_SYMBOL_GPL(dev_iommu_priv_set);
@@ -483,9 +483,9 @@ static int iommu_init_device(struct device *dev)
  	 * found no IOMMU to wait for, so there's no point calling it again.
  	 */
  	if (!dev->iommu->fwspec && !dev->driver && dev->bus->dma_configure) {
-		mutex_unlock(&iommu_probe_device_lock);
+		mutex_unlock(iommu_probe_device_lock(dev));
  		dev->bus->dma_configure(dev);
-		mutex_lock(&iommu_probe_device_lock);
+		mutex_lock(iommu_probe_device_lock(dev));
  		/* If another instance finished the job for us, skip it */
  		if (!dev->iommu || dev->iommu_group)
  			return -ENODEV;
@@ -622,7 +622,20 @@ static struct iommu_domain *pasid_array_entry_to_domain(void *entry)
  	return ((struct iommu_attach_handle *)xa_untag_pointer(entry))->domain;
  }
  
-DEFINE_MUTEX(iommu_probe_device_lock);
+static struct mutex __iommu_probe_device_lock[4] = {
+	__MUTEX_INITIALIZER(iommu_probe_device_lock),
+	__MUTEX_INITIALIZER(iommu_probe_device_lock),
+	__MUTEX_INITIALIZER(iommu_probe_device_lock),
+	__MUTEX_INITIALIZER(iommu_probe_device_lock),
+};
+
+struct mutex *iommu_probe_device_lock(const struct device *dev)
+{
+	int hash = ((uintptr_t)dev / roundup_pow_of_two(sizeof(*dev))) %
+		    ARRAY_SIZE(__iommu_probe_device_lock);
+
+	return __iommu_probe_device_lock + hash;
+}
  
  static int __iommu_probe_device(struct device *dev, struct list_head *group_list)
  {
@@ -637,7 +650,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
  	 * probably be able to use device_lock() here to minimise the scope,
  	 * but for now enforcing a simple global ordering is fine.
  	 */
-	lockdep_assert_held(&iommu_probe_device_lock);
+	lockdep_assert_held(iommu_probe_device_lock(dev));
  
  	/* Device is probed already if in a group */
  	if (dev->iommu_group)
@@ -711,9 +724,9 @@ int iommu_probe_device(struct device *dev)
  	const struct iommu_ops *ops;
  	int ret;
  
-	mutex_lock(&iommu_probe_device_lock);
+	mutex_lock(iommu_probe_device_lock(dev));
  	ret = __iommu_probe_device(dev, NULL);
-	mutex_unlock(&iommu_probe_device_lock);
+	mutex_unlock(iommu_probe_device_lock(dev));
  	if (ret)
  		return ret;
  
@@ -1803,9 +1816,9 @@ static int probe_iommu_group(struct device *dev, void *data)
  	struct list_head *group_list = data;
  	int ret;
  
-	mutex_lock(&iommu_probe_device_lock);
+	mutex_lock(iommu_probe_device_lock(dev));
  	ret = __iommu_probe_device(dev, group_list);
-	mutex_unlock(&iommu_probe_device_lock);
+	mutex_unlock(iommu_probe_device_lock(dev));
  	if (ret == -ENODEV)
  		ret = 0;
  
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index a18bb60f6f3d..b5e3a425ca2d 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -121,9 +121,9 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
  		return -ENODEV;
  
  	/* Serialise to make dev->iommu stable under our potential fwspec */
-	mutex_lock(&iommu_probe_device_lock);
+	mutex_lock(iommu_probe_device_lock(dev));
  	if (dev_iommu_fwspec_get(dev)) {
-		mutex_unlock(&iommu_probe_device_lock);
+		mutex_unlock(iommu_probe_device_lock(dev));
  		return 0;
  	}
  	dev_iommu_present = dev->iommu;
@@ -151,7 +151,7 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
  		iommu_fwspec_free(dev);
  	else if (err && dev->iommu)
  		dev_iommu_free(dev);
-	mutex_unlock(&iommu_probe_device_lock);
+	mutex_unlock(iommu_probe_device_lock(dev));
  
  	/*
  	 * If we're not on the iommu_probe_device() path (as indicated by the
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ac43b8b93f14..3b876cb285e0 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1201,7 +1201,7 @@ static inline void *dev_iommu_priv_get(struct device *dev)
  
  void dev_iommu_priv_set(struct device *dev, void *priv);
  
-extern struct mutex iommu_probe_device_lock;
+struct mutex *iommu_probe_device_lock(const struct device *dev);
  int iommu_probe_device(struct device *dev);
  
  int iommu_device_use_default_domain(struct device *dev);
-- 
2.54.0.dirty