Re: [PATCH V1] iommu/sva: Fix crash in iommu_sva_unbind_device()

[Lizhi Hou <lizhi.hou@amd.com>]

On 2/25/26 01:35, Yi Liu wrote:
> [You don't often get email from yi.l.liu@intel.com. Learn why this is 
> important at https://aka.ms/LearnAboutSenderIdentification ]
>
> On 2026/2/25 02:30, Lizhi Hou wrote:
>> domain->mm->iommu_mm can be freed by iommu_domain_free():
>>    iommu_domain_free()
>>      mmdrop()
>>        __mmdrop()
>>          mm_pasid_drop()
>> After iommu_domain_free() returns, accessing domain->mm->iommu_mm may
>> dereference a freed mm structure, leading to a crash.
>>
>> Fix this by taking a reference to the mm via mmgrab() before
>> calling iommu_domain_free(), and dropping it with mmdrop() after
>> finishing access to domain->mm->iommu_mm.
>
> need to be more accurate. The issue is that iommu_mm is freed in
> mm_pasid_drop(), so after iommu_domain_free(), the later access to
> iommu_mm is problematic.

The description mentioned this.

After iommu_domain_free() returns, accessing domain->mm->iommu_mm may
dereference a freed mm structure, leading to a crash.

>
>> Fixes: e37d5a2d60a3 ("iommu/sva: invalidate stale IOTLB entries for 
>> kernel address space")
>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>> ---
>>   drivers/iommu/iommu-sva.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
>> index 07d64908a05f..523b8c65c86f 100644
>> --- a/drivers/iommu/iommu-sva.c
>> +++ b/drivers/iommu/iommu-sva.c
>> @@ -179,6 +179,7 @@ void iommu_sva_unbind_device(struct iommu_sva 
>> *handle)
>>               return;
>>       }
>>
>> +     mmgrab(domain->mm);
>>       iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
>>       if (--domain->users == 0) {
>>               list_del(&domain->next);
>> @@ -190,6 +191,7 @@ void iommu_sva_unbind_device(struct iommu_sva 
>> *handle)
>>               if (list_empty(&iommu_sva_mms))
>>                       iommu_sva_present = false;
>>       }
>> +     mmdrop(domain->mm);
>>
>>       mutex_unlock(&iommu_sva_lock);
>>       kfree(handle);
>
> will moving the below hunk in front of iommu_domain_free() simpler?
> Only when (--domain->users == 0), shall the code check if sva_domains
> is empty. right?

I am not sure if this can be moved in front of iommu_domain_free(). Will 
iommu_domain_free() be possible to impact sva_domains?

iommu_domain_free() calls domain->ops->free(). Could this call back free 
sva_domain?


Lizhi

>
>         if (list_empty(&iommu_mm->sva_domains)) {
>             list_del(&iommu_mm->mm_list_elm);
>             if (list_empty(&iommu_sva_mms))
>                 iommu_sva_present = false;
>         }
>
> Regards,
> Yi Liu