* [PATCH] vfio/iommu_type1: acquire iommu lock in vfio_iommu_type1_release()
@ 2023-06-07 19:07 Sidhartha Kumar
2023-06-07 19:40 ` Alex Williamson
0 siblings, 1 reply; 4+ messages in thread
From: Sidhartha Kumar @ 2023-06-07 19:07 UTC (permalink / raw)
To: linux-kernel, kvm, alex.williamson; +Cc: khalid.aziz, Sidhartha Kumar
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. 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@oracle.com>
Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
---
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) {
--
2.40.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] vfio/iommu_type1: acquire iommu lock in vfio_iommu_type1_release()
2023-06-07 19:07 [PATCH] vfio/iommu_type1: acquire iommu lock in vfio_iommu_type1_release() Sidhartha Kumar
@ 2023-06-07 19:40 ` Alex Williamson
2023-06-08 20:16 ` Sidhartha Kumar
0 siblings, 1 reply; 4+ messages in thread
From: Alex Williamson @ 2023-06-07 19:40 UTC (permalink / raw)
To: Sidhartha Kumar; +Cc: linux-kernel, kvm, khalid.aziz
On Wed, 7 Jun 2023 12:07:52 -0700
Sidhartha Kumar <sidhartha.kumar@oracle.com> 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,
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@oracle.com>
> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
> 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) {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] vfio/iommu_type1: acquire iommu lock in vfio_iommu_type1_release()
2023-06-07 19:40 ` Alex Williamson
@ 2023-06-08 20:16 ` Sidhartha Kumar
2023-06-08 20:56 ` Alex Williamson
0 siblings, 1 reply; 4+ messages in thread
From: Sidhartha Kumar @ 2023-06-08 20:16 UTC (permalink / raw)
To: Alex Williamson; +Cc: linux-kernel, kvm, khalid.aziz
On 6/7/23 12:40 PM, Alex Williamson wrote:
> On Wed, 7 Jun 2023 12:07:52 -0700
> Sidhartha Kumar <sidhartha.kumar@oracle.com> 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@oracle.com>
>> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>> ---
>> 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) {
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] vfio/iommu_type1: acquire iommu lock in vfio_iommu_type1_release()
2023-06-08 20:16 ` Sidhartha Kumar
@ 2023-06-08 20:56 ` Alex Williamson
0 siblings, 0 replies; 4+ messages in thread
From: Alex Williamson @ 2023-06-08 20:56 UTC (permalink / raw)
To: Sidhartha Kumar; +Cc: linux-kernel, kvm, khalid.aziz
On Thu, 8 Jun 2023 13:16:12 -0700
Sidhartha Kumar <sidhartha.kumar@oracle.com> wrote:
> On 6/7/23 12:40 PM, Alex Williamson wrote:
> > On Wed, 7 Jun 2023 12:07:52 -0700
> > Sidhartha Kumar <sidhartha.kumar@oracle.com> 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.
Ok, I think we're going to need to wait for that root cause. It's
trivial to hold the lock here, but we're imminently freeing the object
hosting that lock, so we have bigger problems than threads stepping on
each other in the pfn_list if this lock were to actually resolve
anything. It's not an oversight that we don't hold the lock here,
there should be no outstanding references to the iommu object at this
point. Thanks,
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@oracle.com>
> >> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> >> ---
> >> 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) {
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-08 20:57 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-07 19:07 [PATCH] vfio/iommu_type1: acquire iommu lock in vfio_iommu_type1_release() Sidhartha Kumar
2023-06-07 19:40 ` Alex Williamson
2023-06-08 20:16 ` Sidhartha Kumar
2023-06-08 20:56 ` Alex Williamson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).