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

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

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