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

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

Hi Baolu,


On 6/14/2023 11:57 AM, Baolu Lu wrote:
> 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.

Yeah. I have seen those changes but forgot while writing commit message. Will
fix it.


> 
>>    - 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?

I think we should keep below change.


> 
>> -    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.

Sure. Will fix it.

Thanks
-Vasant