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

[sashiko-bot@kernel.org]

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