Re: [PATCH] iommu: iommu_group_claim_dma_owner() must always assign a domain

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

On 2022-05-04 01:11, Jason Gunthorpe wrote:
> Once the group enters 'owned' mode it can never be assigned back to the
> default_domain or to a NULL domain. It must always be actively assigned to
> a current domain. If the caller hasn't provided a domain then the core
> must provide an explicit DMA blocking domain that has no DMA map.
> 
> Lazily create a group-global blocking DMA domain when
> iommu_group_claim_dma_owner is first called and immediately assign the
> group to it. This ensures that DMA is immediately fully isolated on all
> IOMMU drivers.
> 
> If the user attaches/detaches while owned then detach will set the group
> back to the blocking domain.
> 
> Slightly reorganize the call chains so that
> __iommu_group_attach_core_domain() is the function that removes any caller
> configured domain and sets the domains back a core owned domain with an
> appropriate lifetime.
> 
> __iommu_group_attach_domain() is the worker function that can change the
> domain assigned to a group to any target domain, including NULL.
> 
> Add comments clarifying how the NULL vs detach_dev vs default_domain works
> based on Robin's remarks.
> 
> This fixes an oops with VFIO and SMMUv3 because VFIO will call
> iommu_detach_group() and then immediately iommu_domain_free(), but
> SMMUv3 has no way to know that the domain it is holding a pointer to
> has been freed. Now the iommu_detach_group() will assign the blocking
> domain and SMMUv3 will no longer hold a stale domain reference.

Thanks for taking this on! I do like the overall structure and naming 
much more than my initial sketch :)

> Fixes: 1ea2a07a532b ("iommu: Add DMA ownership management interfaces")
> Reported-by: Qian Cai <quic_qiancai@quicinc.com>

Co-developed-by: Robin Murphy <robin.murphy@arm.com>

(so the sign-off chain makes sense - you deserve overall authorship here)

> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
[...]
> @@ -2072,38 +2072,66 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
>   	return 0;
>   }
>   
> -static void __iommu_detach_group(struct iommu_domain *domain,
> -				 struct iommu_group *group)
> +static int __iommu_group_attach_domain(struct iommu_group *group,
> +				       struct iommu_domain *new_domain)
>   {
>   	int ret;
>   
> +	if (group->domain == new_domain)
> +		return 0;
> +
>   	/*
> -	 * If the group has been claimed already, do not re-attach the default
> -	 * domain.
> +	 * A NULL domain means to call the detach_dev() op. New drivers should
> +	 * use a IOMMU_DOMAIN_IDENTITY domain instead of a NULL default_domain

Nit: IOMMU_DOMAIN_DMA is the baseline of default domain support, 
passthrough is more of an optional extra.

> +	 * and detatch_dev().

Nit: detach_dev

Otherwise, modulo the other things already pointed out, this looks OK to me.

Cheers,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu