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

[sashiko-bot@kernel.org]

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