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@xxxxxxx>
> ---
>   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