Re: [PATCH] vfio/iommu_type1: acquire iommu lock in vfio_iommu_type1_release()

[Sidhartha Kumar]

On 6/7/23 12:40 PM, Alex Williamson wrote:
> On Wed,  7 Jun 2023 12:07:52 -0700
> Sidhartha Kumar <sidhartha.kumar@xxxxxxxxxx> wrote:
>
> >  From vfio_iommu_type1_release() there is a code path:
> >
> > vfio_iommu_unmap_unpin_all()
> >   vfio_remove_dma()
> >      vfio_unmap_unpin()
> >        unmap_unpin_slow()
> >          vfio_unpin_pages_remote()
> >            vfio_find_vpfn()
> >
> > This path is taken without acquiring the iommu lock so it could lead to
> > a race condition in the traversal of the pfn_list rb tree.
>
> What's the competing thread for the race, vfio_remove_dma() tests:
>
> 	WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list));
>
> The fix is not unreasonable, but is this a theoretical fix upstream
> that's tickled by some downstream additions, or are we actually
> competing against page pinning by an mdev driver after the container is
> released?  Thanks,

Hello,

In a stress test of starting and stopping multiple VMs for a few days we 
observed a memory leak that occurs after a few days. These guests have 
their memory pinned via the pin_user_pages_remote() call in 
vaddr_get_pfns(). From examining the vfio/iommu_type1 code this 
potential race condition was noticed, but we have not root caused this 
race to be the cause of the memory leak.

Thanks,
Sidhartha Kumar
> Alex
>
> > The lack of
> > the iommu lock in vfio_iommu_type1_release() was confirmed by adding a
> >
> > WARN_ON(!mutex_is_locked(&iommu->lock))
> >
> > which was reported in dmesg. Fix this potential race by adding a iommu
> > lock and release in vfio_iommu_type1_release().
> >
> > Suggested-by: Khalid Aziz <khalid.aziz@xxxxxxxxxx>
> > Signed-off-by: Sidhartha Kumar <sidhartha.kumar@xxxxxxxxxx>
> > ---
> >   drivers/vfio/vfio_iommu_type1.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> > index 306e6f1d1c70e..7d2fea1b483dc 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -2601,7 +2601,9 @@ static void vfio_iommu_type1_release(void *iommu_data)
> >   		kfree(group);
> >   	}
> >   
> > +	mutex_lock(&iommu->lock);
> >   	vfio_iommu_unmap_unpin_all(iommu);
> > +	mutex_unlock(&iommu->lock);
> >   
> >   	list_for_each_entry_safe(domain, domain_tmp,
> >   				 &iommu->domain_list, next) {