Re: [PATCH v2 iommu/next] iommu: Fix default domain setup

[Baolu Lu <baolu.lu@linux.intel.com>]

On 6/19/23 4:49 PM, Vasant Hegde wrote:
> Commit 1000dccd5d13 ("iommu: Allow IOMMU_RESV_DIRECT to work on ARM")
> accidently restored "group->domain" to "old_domain" while keeping
> "group->default_domain" to new domain. Also freed new domain.
> 
> This works fine during boot as 'old_domain' is NULL. But if we try
> change domain via sysfs using below command then kernel crashes with
> "kernel NULL pointer dereference".
> 
> Change domain command :
>    - Go to /sys/kernel/iommu_groups/<group id>/
>    - Unbind device driver for all devices in the group
>    - echo "<dom type>" > /sys/kernel/iommu_groups/<group id>/type
> 
> Fix above described issue by handling error path properly.
> 
> Reported-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
> Fixes: 1000dccd5d13 ("iommu: Allow IOMMU_RESV_DIRECT to work on ARM")
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Tested-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
> Changes in v2:
>    - Addressed review comments from Baolu
> 
> @Joerg,
>    This patch applies on top of iommu/next branch.
> 
> -Vasant
>   drivers/iommu/iommu.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 9e0228ef612b..e74e45f42c75 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2915,10 +2915,15 @@ static int iommu_setup_default_domain(struct iommu_group *group,
>   		}
>   	}
>   
> +	if (old_dom)
> +		iommu_domain_free(old_dom);
> +	return ret;

Add out_free tag here and remove the duplicate code at the end of this
function.

> +
>   err_restore:
>   	if (old_dom) {
>   		__iommu_group_set_domain_internal(
>   			group, old_dom, IOMMU_SET_DOMAIN_MUST_SUCCEED);
> +		group->default_domain = old_dom;
>   		iommu_domain_free(dom);
>   		old_dom = NULL;
>   	}

The err_restore branch doesn't work if old_dom is NULL. We have no means
to restore a group from a successful first-time attaching to NULL
attaching.

Or I overlooked anything?

Best regards,
baolu