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

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