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

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

On 6/14/23 1:56 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 :
>    - Assumed we have single device in the IOMMU group

Nit: The requirement for singleton group has been removed.

>    - unbind device driver
>    - 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")
> Tested-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
> Hi Joerg,
>    This patch is on top of iommu/next branch.
> 
> -Vasant
> 
>   drivers/iommu/iommu.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 9e0228ef612b..d21d0a217bdf 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2880,7 +2880,6 @@ static int iommu_setup_default_domain(struct iommu_group *group,
>   	}
>   
>   	/* We must set default_domain early for __iommu_device_set_domain */

Is the above comment still relevant? If yes, this fix seems to be
problematic. Or no?

> -	group->default_domain = dom;
>   	if (!group->domain) {
>   		/*
>   		 * Drivers are not allowed to fail the first domain attach.
> @@ -2896,7 +2895,6 @@ static int iommu_setup_default_domain(struct iommu_group *group,
>   		ret = __iommu_group_set_domain(group, dom);
>   		if (ret) {
>   			iommu_domain_free(dom);
> -			group->default_domain = old_dom;
>   			return ret;
>   		}
>   	}
> @@ -2915,16 +2913,20 @@ static int iommu_setup_default_domain(struct iommu_group *group,
>   		}
>   	}
>   
> +out_free:
> +	group->default_domain = dom;
> +
> +	if (old_dom) {
> +		iommu_domain_free(old_dom);
> +		old_dom = NULL;
> +	}
> +
>   err_restore:
>   	if (old_dom) {
>   		__iommu_group_set_domain_internal(
>   			group, old_dom, IOMMU_SET_DOMAIN_MUST_SUCCEED);
>   		iommu_domain_free(dom);
> -		old_dom = NULL;
>   	}
> -out_free:
> -	if (old_dom)
> -		iommu_domain_free(old_dom);
>   	return ret;
>   }
>   

It's better to split the error path from the successful one.

Best regards,
baolu