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

[Jason Gunthorpe <jgg@nvidia.com>]

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..

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.
 
> 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