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

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

On 3/4/26 02:10, 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 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?

Ok, I searched the code more. Yes, you are correct. I will generate 
patch V2.

Lizhi

>
> Regards,
> Yi Liu
>