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

[Peter Chen]

On 26-09-29 00:07:37, Peter Chen wrote:
> 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@xxxxxxxxxxxxxxxx>
> ---
>  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
> 

A gentle ping.

-- 

Thanks,
Peter Chen