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

[Yi Liu <yi.l.liu@intel.com>]

On 3/4/26 02:33, Lizhi Hou wrote:
> 
> On 2/28/26 04:14, Yi Liu wrote:
>>
>> On 2/26/26 06:09, Lizhi Hou wrote:
>>>
>>>>> 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?
>>
>> sva_domains is used to track domains associated with the same mm in the
>> generic layer. iommu_domain_free() calls vendor iommu driver's free() op
>> with domain type specific operations. I don't think it should impact the
>> sva_domains.
>>
>>> iommu_domain_free() calls domain->ops->free(). Could this call back free
>>> sva_domain?
>>
>> I think so. Check intel_mm_free_notifier() as an example.
> 
> So if this is true, after calling iommu_domain_free(), the following 
> check list_empty(&iommu_mm->sva_domains) could become to true because of 
> domain free. If we move the check in front (before iommu_domain_free()), 
> the check could be false. That seems leading incorrect iommu_sva_present

list_empty(&iommu_mm->sva_domains) can be true because of 
list_del(&domain->next), not iommu_domain_free(). Am I missing
anything here?

Regards,
Yi Liu