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

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

[Lizhi Hou]

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.

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);
-- 
2.34.1

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

[Yi Liu]

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.

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

         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

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

[Lizhi Hou]

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

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

[Yi Liu]

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.

Regards,
Yi Liu

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

[Baolu Lu]

On 2/25/26 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.
> 
> 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);

How about making the iommu_mm structure itself an owner of the mm_struct
lifetime? Does something like the following work?

diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 07d64908a05f..9c56c2222617 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -47,6 +47,7 @@ static struct iommu_mm_data 
*iommu_alloc_mm_data(struct mm_struct *mm, struct de
         iommu_mm->pasid = pasid;
         iommu_mm->mm = mm;
         INIT_LIST_HEAD(&iommu_mm->sva_domains);
+       mmgrab(mm);
         /*
          * Make sure the write to mm->iommu_mm is not reordered in front of
          * initialization to iommu_mm fields. If it does, readers may see a
@@ -212,6 +213,8 @@ void mm_pasid_drop(struct mm_struct *mm)
                 return;

         iommu_free_global_pasid(iommu_mm->pasid);
+       mm->iommu_mm = NULL;
+       mmdrop(mm);
         kfree(iommu_mm);
  }

Thanks,
baolu

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

[Yi Liu]

On 3/2/26 14:50, Baolu Lu wrote:
> On 2/25/26 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.
>>
>> 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);

Hi Baolu,

> How about making the iommu_mm structure itself an owner of the mm_struct
> lifetime? Does something like the following work?

According to the call stack in the commit message, mm_pasid_drop() is
triggered when the mm_count reaches 0.

 >>      mmdrop()
 >>        __mmdrop()
 >>          mm_pasid_drop()

So I see a deadlock issue with the below change. The problem is:

mmgrab(mm) in iommu_alloc_mm_data() increases mm_count. But 
mm_pasid_drop() calls mmdrop(mm) to decrease mm_count. This creates a
circular dependency: __mmdrop() waits for mm_count to be 0, but mm_count
can only reach 0 after __mmdrop() calls mm_pasid_drop().

> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 07d64908a05f..9c56c2222617 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -47,6 +47,7 @@ static struct iommu_mm_data 
> *iommu_alloc_mm_data(struct mm_struct *mm, struct de
>          iommu_mm->pasid = pasid;
>          iommu_mm->mm = mm;
>          INIT_LIST_HEAD(&iommu_mm->sva_domains);
> +       mmgrab(mm);
>          /*
>           * Make sure the write to mm->iommu_mm is not reordered in 
> front of
>           * initialization to iommu_mm fields. If it does, readers may 
> see a
> @@ -212,6 +213,8 @@ void mm_pasid_drop(struct mm_struct *mm)
>                  return;
> 
>          iommu_free_global_pasid(iommu_mm->pasid);
> +       mm->iommu_mm = NULL;
> +       mmdrop(mm);


Regards,
Yi Liu

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

[Lizhi Hou]

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

Lizhi

>
> Regards,
> Yi Liu
>

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

[Yi Liu]

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

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

[Lizhi Hou]

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
>