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

[sashiko-bot@kernel.org]

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