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

[Vasant Hegde <vasant.hegde@amd.com>]

Hi Jason,


On 6/19/2023 5:01 PM, Jason Gunthorpe wrote:
> On Mon, Jun 19, 2023 at 08:49:45AM +0000, 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".
> 
> There is more wrong here then.. The error unwind should work even if
> we do it at the wrong time.

Agree. Unwind should work all time. But I'm not sure what's wrong with my patch.
Are you saying you prefer `goto` in all error path?


> 
> Like this?

This also fine.

-Vasant


> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 8f3464ba204498..6fb4533905c37b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2960,14 +2960,11 @@ static int iommu_setup_default_domain(struct iommu_group *group,
>  		ret = __iommu_group_set_domain_internal(
>  			group, dom, IOMMU_SET_DOMAIN_MUST_SUCCEED);
>  		if (WARN_ON(ret))
> -			goto out_free;
> +			goto out_free_old;
>  	} else {
>  		ret = __iommu_group_set_domain(group, dom);
> -		if (ret) {
> -			iommu_domain_free(dom);
> -			group->default_domain = old_dom;
> -			return ret;
> -		}
> +		if (ret)
> +			goto err_restore_def_domain;
>  	}
>  
>  	/*
> @@ -2980,21 +2977,25 @@ static int iommu_setup_default_domain(struct iommu_group *group,
>  		for_each_group_device(group, gdev) {
>  			ret = iommu_create_device_direct_mappings(dom, gdev->dev);
>  			if (ret)
> -				goto err_restore;
> +				goto err_restore_domain;
>  		}
>  	}
>  
> -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:
> +out_free_old:
>  	if (old_dom)
>  		iommu_domain_free(old_dom);
>  	return ret;
> +
> +err_restore_domain:
> +	if (old_dom)
> +		__iommu_group_set_domain_internal(
> +			group, old_dom, IOMMU_SET_DOMAIN_MUST_SUCCEED);
> +err_restore_def_domain:
> +	if (old_dom) {
> +		iommu_domain_free(dom);
> +		group->default_domain = old_dom;
> +	}
> +	return ret;
>  }
>  
>  /*