Re: [PATCH v8 1/5] iommu/arm-smmu: Destroy domain context in failure path

[Vivek Gautam]

On Wed, Mar 7, 2018 at 5:50 PM, Robin Murphy  wrote:
> On 02/03/18 10:10, Vivek Gautam wrote:
>>
>> If we fail after initializing domain_context, we should destroy
>> the context to free up resources.
>
>
> Have another think about why the "problem" this patch caters for cannot ever
> happen (hint: consider how domain->smmu is used in
> arm_smmu_init_domain_context()). And then also about the really
> catastrophically bad problem it actually introduces (hint:
> "iommu_attach(domain, good_dev); iommu_attach(domain, bad_dev);")

Got it, we would end up destroying good_dev's domain context with this.
Thanks

regards
Vivek

>
> Robin.
>
>
>> Signed-off-by: Vivek Gautam 
>> ---
>>
>>   * New patch added in this series.
>>
>>   drivers/iommu/arm-smmu.c | 7 ++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 69e7c60792a8..ffc152c36002 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1223,11 +1223,16 @@ static int arm_smmu_attach_dev(struct iommu_domain
>> *domain, struct device *dev)
>> dev_err(dev,
>> "cannot attach to SMMU %s whilst already attached
>> to domain on SMMU %s\n",
>> dev_name(smmu_domain->smmu->dev),
>> dev_name(smmu->dev));
>> -   return -EINVAL;
>> +   ret = -EINVAL;
>> +   goto destroy_domain;
>> }
>> /* Looks ok, so add the device to the domain */
>> return arm_smmu_domain_add_master(smmu_domain, fwspec);
>> +
>> +destroy_domain:
>> +   arm_smmu_destroy_domain_context(domain);
>> +   return ret;
>>   }
>> static int arm_smmu_map(struct iommu_domain *domain, unsigned long
>> iova,
>>
>



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v8 1/5] iommu/arm-smmu: Destroy domain context in failure path

[Robin Murphy]

On 02/03/18 10:10, Vivek Gautam wrote:

If we fail after initializing domain_context, we should destroy
the context to free up resources.


Have another think about why the "problem" this patch caters for cannot 
ever happen (hint: consider how domain->smmu is used in 
arm_smmu_init_domain_context()). And then also about the really 
catastrophically bad problem it actually introduces (hint: 
"iommu_attach(domain, good_dev); iommu_attach(domain, bad_dev);")


Robin.


Signed-off-by: Vivek Gautam 
---

  * New patch added in this series.

  drivers/iommu/arm-smmu.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 69e7c60792a8..ffc152c36002 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1223,11 +1223,16 @@ static int arm_smmu_attach_dev(struct iommu_domain 
*domain, struct device *dev)
dev_err(dev,
"cannot attach to SMMU %s whilst already attached to domain 
on SMMU %s\n",
dev_name(smmu_domain->smmu->dev), dev_name(smmu->dev));
-   return -EINVAL;
+   ret = -EINVAL;
+   goto destroy_domain;
}
  
  	/* Looks ok, so add the device to the domain */

return arm_smmu_domain_add_master(smmu_domain, fwspec);
+
+destroy_domain:
+   arm_smmu_destroy_domain_context(domain);
+   return ret;
  }
  
  static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,



___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu