Re: [RFC PATCH kernel] vfio/spapr_tce: Get rid of possible infinite loop
Alexey Kardashevskiy wrote:
> As a part of cleanup, the SPAPR TCE IOMMU subdriver releases preregistered
> memory. If there is a bug in memory release, the loop in
> tce_iommu_release() becomes infinite; this actually happened to me.
>
> This makes the loop finite and prints a warning on every failure to make
> the code more bug prone.
>
> Signed-off-by: Alexey Kardashevskiy
> ---
> drivers/vfio/vfio_iommu_spapr_tce.c | 10 +++---
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c
> b/drivers/vfio/vfio_iommu_spapr_tce.c
> index b1a8ab3..ece0651 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -371,6 +371,7 @@ static void tce_iommu_release(void *iommu_data)
> {
> struct tce_container *container = iommu_data;
> struct tce_iommu_group *tcegrp;
> + struct tce_iommu_prereg *tcemem, *tmtmp;
> long i;
>
> while (tce_groups_attached(container)) {
> @@ -393,13 +394,8 @@ static void tce_iommu_release(void *iommu_data)
> tce_iommu_free_table(container, tbl);
> }
>
> - while (!list_empty(>prereg_list)) {
> - struct tce_iommu_prereg *tcemem;
> -
> - tcemem = list_first_entry(>prereg_list,
> - struct tce_iommu_prereg, next);
> - WARN_ON_ONCE(tce_iommu_prereg_free(container, tcemem));
> - }
> + list_for_each_entry_safe(tcemem, tmtmp, >prereg_list, next)
> + WARN_ON(tce_iommu_prereg_free(container, tcemem));
I'm not sure that tce_iommu_prereg_free() call under WARN_ON() is good
idea because WARN_ON() is a preprocessor macro:
if CONFIG_WARN=n is added by the analogy with CONFIG_BUG=n defining
WARN_ON() as empty we will loose call to tce_iommu_prereg_free()
leaking resources.
There is no problem at the moment: WARN_ON() defined for PPC in
arch/powerpc/include/asm/bug.h unconditionally.
So your first version with intermediate variable looks better to me.
>
> tce_iommu_disable(container);
> if (container->mm)
>
--
Thanks,
Serhii
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 0/4] Fix use after free in HPT resizing code and related minor improvements
David Gibson wrote: > On Wed, Nov 29, 2017 at 11:38:22AM -0500, Serhii Popovych wrote: >> It is possible to trigger use after free during HPT resize >> causing host kernel to crash. More details and analysis of >> the problem can be found in change with corresponding subject >> (KVM: PPC: Book3S HV: Fix use after free in case of multiple >> resize requests). >> >> We need some changes to prepare for the fix, especially >> make ->error in HPT resize instance single point for >> tracking allocation state, improve kvmppc_allocate_hpt() >> and kvmppc_free_hpt() so they can be used more safely. >> >> See individual commit description message to get more >> information on changes presented. > > I spoke with Paul Mackerras about these patches on IRC today. We want > this as a fix, ASAP, in 4.15. However, he's uncomfortable with > pushing some of extra cleanups which aren't necessary for the bug fix > this late for 4.15, and was having trouble following what was the core > of the fix. He was also nervous about the addition of more BUG_ON()s. Good, no problem, cleanups will be pushed additionally. > > To avoid the round trip to Ukraine time and back, I've made revised > versions of patches 1 & 3 which should apply standalone, replaced the > BUG_ON()s with WARN_ON()s and revised the commit messages to better > explain the crucial part of the fix. > > However, I've run out of time to test them. I did the same test as for this v1 series and found no problem with v2 you sent to me: it seems patch improving kvmppc_allocate_hpt() and kvmppc_free_hpt() isn't actually necessary as I was thinking when submitting v1. > > Serhii, I'll send you my revised patches shortly. Can you please > test them and repost. Then you can rebase patches 2 & 4 from this > series on top of the revised patches and post those separately (as a > cleanup with less urgency than the actual fix). Tested with same test case as with v1: no problem so far. > > A couple of people have also suggested CCing k...@vger.kernel.org on > the next round in addition to the lists already included. > Done. -- Thanks, Serhii signature.asc Description: OpenPGP digital signature
[PATCH v2 2/2] KVM: PPC: Book3S HV: Fix use after free in case of multiple resize requests
.280426] [c007e9183ba0] [dd24da04]
resize_hpt_prepare_work+0xe4/0x1c0 [kvm_hv]
[ 635.280507] [c007e9183c40] [c0113c0c]
process_one_work+0x1dc/0x680
[ 635.280587] [c007e9183ce0] [c0114250] worker_thread+0x1a0/0x520
[ 635.280655] [c007e9183d80] [c012010c] kthread+0xec/0x100
[ 635.280724] [c007e9183e30] [c000a4b8]
ret_from_kernel_thread+0x5c/0xa4
[ 635.280814] Instruction dump:
[ 635.280880] 7c0802a6 fba1ffe8 fbc1fff0 7cbd2b78 fbe1fff8 7c9e2378 7c7f1b78
f8010010
[ 635.281099] f821ff81 e8a50008 7fa52040 40de00b8 7fbd2840 40de008c
7fbff040
[ 635.281324] ---[ end trace b628b73449719b9d ]---
Signed-off-by: Serhii Popovych <spopo...@redhat.com>
[dwg: Replaced BUG_ON()s with WARN_ONs() and reworded commit message
for clarity]
Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 50 ++---
1 file changed, 35 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 31fa710..7948b84 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -1419,16 +1419,20 @@ static void resize_hpt_pivot(struct kvm_resize_hpt
*resize)
static void resize_hpt_release(struct kvm *kvm, struct kvm_resize_hpt *resize)
{
- BUG_ON(kvm->arch.resize_hpt != resize);
+ if (WARN_ON(!mutex_is_locked(>lock)))
+ return;
if (!resize)
return;
- if (resize->hpt.virt)
- kvmppc_free_hpt(>hpt);
+ if (resize->error != -EBUSY) {
+ if (resize->hpt.virt)
+ kvmppc_free_hpt(>hpt);
+ kfree(resize);
+ }
- kvm->arch.resize_hpt = NULL;
- kfree(resize);
+ if (kvm->arch.resize_hpt == resize)
+ kvm->arch.resize_hpt = NULL;
}
static void resize_hpt_prepare_work(struct work_struct *work)
@@ -1437,26 +1441,42 @@ static void resize_hpt_prepare_work(struct work_struct
*work)
struct kvm_resize_hpt,
work);
struct kvm *kvm = resize->kvm;
- int err;
+ int err = 0;
if (WARN_ON(resize->error != -EBUSY))
return;
- resize_hpt_debug(resize, "resize_hpt_prepare_work(): order = %d\n",
-resize->order);
+ mutex_lock(>lock);
- err = resize_hpt_allocate(resize);
+ /* Request is still current? */
+ if (kvm->arch.resize_hpt == resize) {
+ /* We may request large allocations here:
+* do not sleep with kvm->lock held for a while.
+*/
+ mutex_unlock(>lock);
- /* We have strict assumption about -EBUSY
-* when preparing for HPT resize.
-*/
- if (WARN_ON(err == -EBUSY))
- err = -EINPROGRESS;
+ resize_hpt_debug(resize, "resize_hpt_prepare_work(): order =
%d\n",
+resize->order);
- mutex_lock(>lock);
+ err = resize_hpt_allocate(resize);
+
+ /* We have strict assumption about -EBUSY
+* when preparing for HPT resize.
+*/
+ if (WARN_ON(err == -EBUSY))
+ err = -EINPROGRESS;
+
+ mutex_lock(>lock);
+ /* It is possible that kvm->arch.resize_hpt != resize
+* after we grab kvm->lock again.
+*/
+ }
resize->error = err;
+ if (kvm->arch.resize_hpt != resize)
+ resize_hpt_release(kvm, resize);
+
mutex_unlock(>lock);
}
--
1.8.3.1
[PATCH v2 1/2] KVM: PPC: Book3S HV: Drop prepare_done from struct kvm_resize_hpt and cleanups
Currently the kvm_resize_hpt structure has two fields relevant to the
state of an ongoing resize: 'prepare_done', which indicates whether
the worker thread has completed or not, and 'error' which indicates
whether it was successful or not.
Since the success/failure isn't known until completion, this is
confusingly redundant. This patch consolidates the information into
just the 'error' value: -EBUSY indicates the worked is still in
progress, other negative values indicate (completed) failure, 0
indicates successful completion.
As a bonus this reduces size of struct kvm_resize_hpt by
__alignof__(struct kvm_hpt_info) and saves few bytes of code.
While there correct comment in struct kvm_resize_hpt which references
a non-existent semaphore (leftover from an early draft).
Assert with WARN_ON() in case of HPT allocation thread work runs more
than once for resize request or resize_hpt_allocate() returns -EBUSY
that is treated specially.
Change comparison against zero to make checkpatch.pl happy.
Signed-off-by: Serhii Popovych <spopo...@redhat.com>
[dwg: Changed BUG_ON()s to WARN_ON()s and altered commit message for
clarity]
Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 44 +++--
1 file changed, 27 insertions(+), 17 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 9660972..31fa710 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -65,11 +65,17 @@ struct kvm_resize_hpt {
u32 order;
/* These fields protected by kvm->lock */
+
+ /* Possible values and their usage:
+* <0 an error occurred during allocation,
+ * -EBUSY allocation is in the progress,
+* 0 allocation made successfuly.
+*/
int error;
- bool prepare_done;
- /* Private to the work thread, until prepare_done is true,
-* then protected by kvm->resize_hpt_sem */
+ /* Private to the work thread, until error != -EBUSY,
+* then protected by kvm->lock.
+*/
struct kvm_hpt_info hpt;
};
@@ -1433,15 +1439,23 @@ static void resize_hpt_prepare_work(struct work_struct
*work)
struct kvm *kvm = resize->kvm;
int err;
+ if (WARN_ON(resize->error != -EBUSY))
+ return;
+
resize_hpt_debug(resize, "resize_hpt_prepare_work(): order = %d\n",
resize->order);
err = resize_hpt_allocate(resize);
+ /* We have strict assumption about -EBUSY
+* when preparing for HPT resize.
+*/
+ if (WARN_ON(err == -EBUSY))
+ err = -EINPROGRESS;
+
mutex_lock(>lock);
resize->error = err;
- resize->prepare_done = true;
mutex_unlock(>lock);
}
@@ -1466,14 +1480,12 @@ long kvm_vm_ioctl_resize_hpt_prepare(struct kvm *kvm,
if (resize) {
if (resize->order == shift) {
- /* Suitable resize in progress */
- if (resize->prepare_done) {
- ret = resize->error;
- if (ret != 0)
- resize_hpt_release(kvm, resize);
- } else {
+ /* Suitable resize in progress? */
+ ret = resize->error;
+ if (ret == -EBUSY)
ret = 100; /* estimated time in ms */
- }
+ else if (ret)
+ resize_hpt_release(kvm, resize);
goto out;
}
@@ -1493,6 +1505,8 @@ long kvm_vm_ioctl_resize_hpt_prepare(struct kvm *kvm,
ret = -ENOMEM;
goto out;
}
+
+ resize->error = -EBUSY;
resize->order = shift;
resize->kvm = kvm;
INIT_WORK(>work, resize_hpt_prepare_work);
@@ -1547,16 +1561,12 @@ long kvm_vm_ioctl_resize_hpt_commit(struct kvm *kvm,
if (!resize || (resize->order != shift))
goto out;
- ret = -EBUSY;
- if (!resize->prepare_done)
- goto out;
-
ret = resize->error;
- if (ret != 0)
+ if (ret)
goto out;
ret = resize_hpt_rehash(resize);
- if (ret != 0)
+ if (ret)
goto out;
resize_hpt_pivot(resize);
--
1.8.3.1
[PATCH v2 0/2] Fix use after free in HPT resizing code and related minor improvements
It is possible to trigger use after free during HPT resize causing host kernel to crash. More details and analysis of the problem can be found in change with corresponding subject (KVM: PPC: Book3S HV: Fix use after free in case of multiple resize requests). We need some changes to prepare for the fix, especially make ->error in HPT resize instance single point for tracking allocation state. See individual commit description message to get more information on changes presented. v2: Serhii Popovych: Tested with current 4.15-rc2 as host kernel on P8 with same testcase as for v1: no problems so far. Serhii Popovych (2): KVM: PPC: Book3S HV: Drop prepare_done from struct kvm_resize_hpt and cleanups KVM: PPC: Book3S HV: Fix use after free in case of multiple resize requests arch/powerpc/kvm/book3s_64_mmu_hv.c | 84 + 1 file changed, 57 insertions(+), 27 deletions(-) -- 1.8.3.1
[PATCH 4/4] KVM: PPC: Book3S HV: Remove redundant parameter from resize_hpt_release()
There is no need to pass it explicitly from the caller:
struct kvm_resize_hpt already contains it.
Additional benefit from this change is that BUG_ON()
assertion now checks that mutex is held on kvm instance
associated with resize structure we going to release.
Also kill check for resize being NULL to make code
simpler and we called with resize != NULL in all
places except kvm_vm_ioctl_resize_hpt_commit().
Signed-off-by: Serhii Popovych <spopo...@redhat.com>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 690f061..a74a0ad 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -1415,12 +1415,11 @@ static void resize_hpt_pivot(struct kvm_resize_hpt
*resize)
resize_hpt_debug(resize, "resize_hpt_pivot() done\n");
}
-static void resize_hpt_release(struct kvm *kvm, struct kvm_resize_hpt *resize)
+static void resize_hpt_release(struct kvm_resize_hpt *resize)
{
- BUG_ON(!mutex_is_locked(>lock));
+ struct kvm *kvm = resize->kvm;
- if (!resize)
- return;
+ BUG_ON(!mutex_is_locked(>lock));
if (resize->error != -EBUSY) {
kvmppc_free_hpt(>hpt);
@@ -1469,7 +1468,7 @@ static void resize_hpt_prepare_work(struct work_struct
*work)
resize->error = err;
if (kvm->arch.resize_hpt != resize)
- resize_hpt_release(kvm, resize);
+ resize_hpt_release(resize);
mutex_unlock(>lock);
}
@@ -1499,13 +1498,13 @@ long kvm_vm_ioctl_resize_hpt_prepare(struct kvm *kvm,
if (ret == -EBUSY)
ret = 100; /* estimated time in ms */
else if (ret)
- resize_hpt_release(kvm, resize);
+ resize_hpt_release(resize);
goto out;
}
/* not suitable, cancel it */
- resize_hpt_release(kvm, resize);
+ resize_hpt_release(resize);
}
ret = 0;
@@ -1590,7 +1589,8 @@ long kvm_vm_ioctl_resize_hpt_commit(struct kvm *kvm,
kvm->arch.mmu_ready = 1;
smp_mb();
out_no_hpt:
- resize_hpt_release(kvm, resize);
+ if (resize)
+ resize_hpt_release(resize);
mutex_unlock(>lock);
return ret;
}
--
1.8.3.1
[PATCH 3/4] KVM: PPC: Book3S HV: Fix use after free in case of multiple resize requests
] ---[ end trace b628b73449719b9d ]---
Signed-off-by: Serhii Popovych <spopo...@redhat.com>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 45 ++---
1 file changed, 32 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 3e9abd9..690f061 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -1417,15 +1417,18 @@ static void resize_hpt_pivot(struct kvm_resize_hpt
*resize)
static void resize_hpt_release(struct kvm *kvm, struct kvm_resize_hpt *resize)
{
- BUG_ON(kvm->arch.resize_hpt != resize);
+ BUG_ON(!mutex_is_locked(>lock));
if (!resize)
return;
- kvmppc_free_hpt(>hpt);
+ if (resize->error != -EBUSY) {
+ kvmppc_free_hpt(>hpt);
+ kfree(resize);
+ }
- kvm->arch.resize_hpt = NULL;
- kfree(resize);
+ if (kvm->arch.resize_hpt == resize)
+ kvm->arch.resize_hpt = NULL;
}
static void resize_hpt_prepare_work(struct work_struct *work)
@@ -1434,24 +1437,40 @@ static void resize_hpt_prepare_work(struct work_struct
*work)
struct kvm_resize_hpt,
work);
struct kvm *kvm = resize->kvm;
- int err;
+ int err = 0;
BUG_ON(resize->error != -EBUSY);
- resize_hpt_debug(resize, "resize_hpt_prepare_work(): order = %d\n",
-resize->order);
+ mutex_lock(>lock);
+
+ /* Request is still current? */
+ if (kvm->arch.resize_hpt == resize) {
+ /* We may request large allocations here:
+* do not sleep with kvm->lock held for a while.
+*/
+ mutex_unlock(>lock);
- err = resize_hpt_allocate(resize);
+ resize_hpt_debug(resize, "resize_hpt_prepare_work(): order =
%d\n",
+resize->order);
- /* We have strict assumption about -EBUSY
-* when preparing for HPT resize.
-*/
- BUG_ON(err == -EBUSY);
+ err = resize_hpt_allocate(resize);
- mutex_lock(>lock);
+ /* We have strict assumption about -EBUSY
+* when preparing for HPT resize.
+*/
+ BUG_ON(err == -EBUSY);
+
+ mutex_lock(>lock);
+ /* It is possible that kvm->arch.resize_hpt != resize
+* after we grab kvm->lock again.
+*/
+ }
resize->error = err;
+ if (kvm->arch.resize_hpt != resize)
+ resize_hpt_release(kvm, resize);
+
mutex_unlock(>lock);
}
--
1.8.3.1
[PATCH 2/4] KVM: PPC: Book3S HV: Improve kvmppc_allocate_hpt()/kvmppc_free_hpt()
There are several points of improvements:
1) Make kvmppc_free_hpt() check if allocation is made before attempt
to release. This follows kfree(p) semantics where p == NULL.
2) Return initialized @info parameter from kvmppc_allocate_hpt()
even if allocation fails.
This allows to use kvmppc_free_hpt() in the caller without
checking that preceded kvmppc_allocate_hpt() was successful
p = kmalloc(size, gfp);
kfree(p);
which is correct for both p != NULL and p == NULL. Followup
change will rely on this behaviour.
3) Better code reuse: kvmppc_free_hpt() can be reused on error
path in kvmppc_allocate_hpt() to avoid code duplication.
4) No need to check for !hpt if allocated from CMA: neither
pfn_to_kaddr() nor page_to_pfn() is 0 in case of page != NULL.
Signed-off-by: Serhii Popovych <spopo...@redhat.com>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 54 ++---
1 file changed, 26 insertions(+), 28 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 0534aab..3e9abd9 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -82,47 +82,44 @@ struct kvm_resize_hpt {
int kvmppc_allocate_hpt(struct kvm_hpt_info *info, u32 order)
{
unsigned long hpt = 0;
- int cma = 0;
- struct page *page = NULL;
- struct revmap_entry *rev;
+ int err, cma = 0;
+ struct page *page;
+ struct revmap_entry *rev = NULL;
unsigned long npte;
+ err = -EINVAL;
if ((order < PPC_MIN_HPT_ORDER) || (order > PPC_MAX_HPT_ORDER))
- return -EINVAL;
+ goto out;
+ err = -ENOMEM;
page = kvm_alloc_hpt_cma(1ul << (order - PAGE_SHIFT));
if (page) {
hpt = (unsigned long)pfn_to_kaddr(page_to_pfn(page));
memset((void *)hpt, 0, (1ul << order));
cma = 1;
- }
-
- if (!hpt)
+ } else {
hpt = __get_free_pages(GFP_KERNEL|__GFP_ZERO|__GFP_RETRY_MAYFAIL
|__GFP_NOWARN, order - PAGE_SHIFT);
-
- if (!hpt)
- return -ENOMEM;
+ if (!hpt)
+ goto out;
+ }
/* HPTEs are 2**4 bytes long */
npte = 1ul << (order - 4);
/* Allocate reverse map array */
- rev = vmalloc(sizeof(struct revmap_entry) * npte);
- if (!rev) {
- if (cma)
- kvm_free_hpt_cma(page, 1 << (order - PAGE_SHIFT));
- else
- free_pages(hpt, order - PAGE_SHIFT);
- return -ENOMEM;
- }
-
+ rev = vmalloc(sizeof(*rev) * npte);
+ if (rev)
+ err = 0;
+out:
info->order = order;
info->virt = hpt;
info->cma = cma;
info->rev = rev;
- return 0;
+ if (err)
+ kvmppc_free_hpt(info);
+ return err;
}
void kvmppc_set_hpt(struct kvm *kvm, struct kvm_hpt_info *info)
@@ -190,12 +187,14 @@ void kvmppc_free_hpt(struct kvm_hpt_info *info)
{
vfree(info->rev);
info->rev = NULL;
- if (info->cma)
- kvm_free_hpt_cma(virt_to_page(info->virt),
-1 << (info->order - PAGE_SHIFT));
- else if (info->virt)
- free_pages(info->virt, info->order - PAGE_SHIFT);
- info->virt = 0;
+ if (info->virt) {
+ if (info->cma)
+ kvm_free_hpt_cma(virt_to_page(info->virt),
+1 << (info->order - PAGE_SHIFT));
+ else
+ free_pages(info->virt, info->order - PAGE_SHIFT);
+ info->virt = 0;
+ }
info->order = 0;
}
@@ -1423,8 +1422,7 @@ static void resize_hpt_release(struct kvm *kvm, struct
kvm_resize_hpt *resize)
if (!resize)
return;
- if (resize->hpt.virt)
- kvmppc_free_hpt(>hpt);
+ kvmppc_free_hpt(>hpt);
kvm->arch.resize_hpt = NULL;
kfree(resize);
--
1.8.3.1
[PATCH 1/4] KVM: PPC: Book3S HV: Drop prepare_done from struct kvm_resize_hpt and cleanups
Replace ->prepare_done flag functionality with special handling
of -EBUSY in ->error as indicator that allocation work is running.
Besides cosmetics this reduces size of struct kvm_resize_hpt by
__alignof__(struct kvm_hpt_info) and saves few bytes of code.
While there correct comment in struct kvm_resize_hpt about locking
used to protect access to certain fields.
Assert with BUG_ON() in case of HPT allocation thread work runs
more than once for resize request or resize_hpt_allocate()
returns -EBUSY that is treated specially.
Change comparison against zero to make checkpatch.pl happy.
Signed-off-by: Serhii Popovych <spopo...@redhat.com>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 42 ++---
1 file changed, 25 insertions(+), 17 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 235319c..0534aab 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -65,11 +65,17 @@ struct kvm_resize_hpt {
u32 order;
/* These fields protected by kvm->lock */
+
+ /* Possible values and their usage:
+* <0 an error occurred during allocation,
+ * -EBUSY allocation is in the progress,
+* 0 allocation made successfuly.
+*/
int error;
- bool prepare_done;
- /* Private to the work thread, until prepare_done is true,
-* then protected by kvm->resize_hpt_sem */
+ /* Private to the work thread, until error != -EBUSY,
+* then protected by kvm->lock.
+*/
struct kvm_hpt_info hpt;
};
@@ -1432,15 +1438,21 @@ static void resize_hpt_prepare_work(struct work_struct
*work)
struct kvm *kvm = resize->kvm;
int err;
+ BUG_ON(resize->error != -EBUSY);
+
resize_hpt_debug(resize, "resize_hpt_prepare_work(): order = %d\n",
resize->order);
err = resize_hpt_allocate(resize);
+ /* We have strict assumption about -EBUSY
+* when preparing for HPT resize.
+*/
+ BUG_ON(err == -EBUSY);
+
mutex_lock(>lock);
resize->error = err;
- resize->prepare_done = true;
mutex_unlock(>lock);
}
@@ -1465,14 +1477,12 @@ long kvm_vm_ioctl_resize_hpt_prepare(struct kvm *kvm,
if (resize) {
if (resize->order == shift) {
- /* Suitable resize in progress */
- if (resize->prepare_done) {
- ret = resize->error;
- if (ret != 0)
- resize_hpt_release(kvm, resize);
- } else {
+ /* Suitable resize in progress? */
+ ret = resize->error;
+ if (ret == -EBUSY)
ret = 100; /* estimated time in ms */
- }
+ else if (ret)
+ resize_hpt_release(kvm, resize);
goto out;
}
@@ -1492,6 +1502,8 @@ long kvm_vm_ioctl_resize_hpt_prepare(struct kvm *kvm,
ret = -ENOMEM;
goto out;
}
+
+ resize->error = -EBUSY;
resize->order = shift;
resize->kvm = kvm;
INIT_WORK(>work, resize_hpt_prepare_work);
@@ -1546,16 +1558,12 @@ long kvm_vm_ioctl_resize_hpt_commit(struct kvm *kvm,
if (!resize || (resize->order != shift))
goto out;
- ret = -EBUSY;
- if (!resize->prepare_done)
- goto out;
-
ret = resize->error;
- if (ret != 0)
+ if (ret)
goto out;
ret = resize_hpt_rehash(resize);
- if (ret != 0)
+ if (ret)
goto out;
resize_hpt_pivot(resize);
--
1.8.3.1
[PATCH 0/4] Fix use after free in HPT resizing code and related minor improvements
It is possible to trigger use after free during HPT resize causing host kernel to crash. More details and analysis of the problem can be found in change with corresponding subject (KVM: PPC: Book3S HV: Fix use after free in case of multiple resize requests). We need some changes to prepare for the fix, especially make ->error in HPT resize instance single point for tracking allocation state, improve kvmppc_allocate_hpt() and kvmppc_free_hpt() so they can be used more safely. See individual commit description message to get more information on changes presented. Serhii Popovych (4): KVM: PPC: Book3S HV: Drop prepare_done from struct kvm_resize_hpt and cleanups KVM: PPC: Book3S HV: Improve kvmppc_allocate_hpt()/kvmppc_free_hpt() KVM: PPC: Book3S HV: Fix use after free in case of multiple resize requests KVM: PPC: Book3S HV: Remove redundant parameter from resize_hpt_release() arch/powerpc/kvm/book3s_64_mmu_hv.c | 139 +--- 1 file changed, 82 insertions(+), 57 deletions(-) -- 1.8.3.1
