Re: [PATCH v2 12/14] iommu: Consolidate the default_domain setup to one function

[Robin Murphy <robin.murphy@arm.com>]

On 2023-03-30 16:36, Jason Gunthorpe wrote:
> On Thu, Mar 30, 2023 at 08:37:16PM +0800, Baolu Lu wrote:
>> On 2023/3/30 7:40, Jason Gunthorpe wrote:
>>> +/**
>>> + * iommu_setup_default_domain - Set the default_domain for the group
>>> + * @group: Group to change
>>> + * @target_type: Domain type to set as the default_domain
>>> + *
>>> + * Allocate a default domain and set it as the current domain on the group. If
>>> + * the group already has a default domain it will be changed to the target_type.
>>> + * When target_type is 0 the default domain is selected based on driver and
>>> + * system preferences.
>>> + */
>>> +static int iommu_setup_default_domain(struct iommu_group *group,
>>> +				      int target_type)
>>> +{
>>> +	struct group_device *gdev;
>>> +	struct iommu_domain *dom;
>>> +	struct bus_type *bus =
>>> +		list_first_entry(&group->devices, struct group_device, list)
>>> +			->dev->bus;
>>> +	int ret;
>>> +
>>> +	lockdep_assert_held(&group->mutex);
>>> +
>>> +	target_type = iommu_get_default_domain_type(group, target_type);
>>> +	if (target_type < 0)
>>> +		return -EINVAL;
>>> +
>>> +	if (group->default_domain && group->default_domain->type == target_type)
>>> +		return 0;
>>> +
>>> +	dom = __iommu_domain_alloc(bus, target_type);
>>> +	if (!dom && target_type != IOMMU_DOMAIN_DMA) {
>>> +		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
>>> +		if (dom)
>>> +			pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
>>> +				target_type, group->name);
>>> +	}
>>
>> The background of the code above is that some ARM IOMMU drivers only
>> support DMA mapping domain and do not support identity domain.
>> Therefore, during boot, if the allocation of identity domain fails, a
>> DMA mapping domain is used instead.
> 
> Er, this is doing two things then because it also allows DMA_FQ to
> degrade to just DMA..

Same thing really; generally the point is that any of the more "special" 
default domain types set with the broad brush of Kconfig or command-line 
can cleanly fall back to the basic type wherever it turns out that 
there's a less-capable driver (or, eventually, IOMMU instance) present.

> I changed it like this:
> 
> 	dom = __iommu_domain_alloc(bus, req_type);
> 	if (!dom && !target_type &&
> 	    (req_type == IOMMU_DOMAIN_IDENTITY ||
> 	     req_type == IOMMU_DOMAIN_DMA_FQ)) {
> 		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
> 
> So the auto selection only happens if the target_type is not automatic.

You mean "is automatic"? I'm missing how target_type and req_type are 
related here, but the general principle should be that if we have 
derived the type from iommu_def_domain_type, because we had nothing more 
specific from either a sysfs request or iommu_get_def_domain_type(), 
then a !IOMMU_DOMAIN_DMA failure can retry with IOMMU_DOMAIN_DMA. 
Otherwise any failure should be final.

Strictly that's not quite what the current code does, since 
iommu_alloc_default_domain() is losing the iommu_get_def_domain_type() 
vs. iommu_def_domain_type distinction, but in the former case a driver's 
.def_domain_type should definitely not be returning a type which that 
driver doesn't support, thus allocation failure would only really 
represent unexpected OOM conditions, where the retry would likely fail 
as well, and the whole system is probably dying anyway. If we're 
refactoring the whole lot, though, we may as well make it logically 
consistent.

Thanks,
Robin.

>> However, this does not apply to use cases that change the default domain
>> through sysfs. In such cases, it seems that we should directly return
>> failure (-ENODEV) and tell the user that the iommu driver does not
>> support identity domain.
> 
> And with the fix below it will return ENODEV rather than autoselect DMA.
> 
>>> +	/*
>>> +	 * There are still some drivers which don't support default domains, so
>>> +	 * we ignore the failure and leave group->default_domain NULL.
>>> +	 *
>>> +	 * We assume that the iommu driver starts up the device in
>>> +	 * 'set_platform_dma_ops' mode if it does not support default domains.
>>> +	 */
>>> +	if (!dom) {
>>> +		ret = 0;
>>> +		goto out_set;
>>> +	}
>>
>> Should we call set_platform_dma_ops here?
> 
> It could be done and should be harmless, but the driver is supposed to
> start up in that mode so don't need to explicitly enter it when
> plugging the device.. I kept things as-is
> 
>> The existing default domain
>> (if exists) will be freed below. But the iommu driver doesn't know about
>> this. It probably will create a UAF case?
> 
> This is a bug:
> 
> 	if (!dom) {
> 		/* Once in default_domain mode we never leave */
> 		if (group->default_domain)
> 			return -ENODEV;
> 
> So default_domain is either NULL forever or set to something forever.
> 
> Jason