* [PATCH] KVM: selftests: Fix 32-bit truncation of vm_get_max_gfn()
@ 2021-05-20 21:26 David Matlack
2021-05-20 21:47 ` Peter Xu
2021-05-20 23:41 ` Venkatesh Srinivas
0 siblings, 2 replies; 5+ messages in thread
From: David Matlack @ 2021-05-20 21:26 UTC (permalink / raw)
To: kvm; +Cc: Paolo Bonzini, Andrew Jones, Ben Gardon, Peter Xu, David Matlack
vm_get_max_gfn() casts vm->max_gfn from a uint64_t to an unsigned int,
which causes the upper 32-bits of the max_gfn to get truncated.
Nobody noticed until now likely because vm_get_max_gfn() is only used
as a mechanism to create a memslot in an unused region of the guest
physical address space (the top), and the top of the 32-bit physical
address space was always good enough.
This fix reveals a bug in memslot_modification_stress_test which was
trying to create a dummy memslot past the end of guest physical memory.
Fix that by moving the dummy memslot lower.
Fixes: 52200d0d944e ("KVM: selftests: Remove duplicate guest mode handling")
Signed-off-by: David Matlack <dmatlack@google.com>
---
tools/testing/selftests/kvm/include/kvm_util.h | 2 +-
tools/testing/selftests/kvm/lib/kvm_util.c | 2 +-
.../testing/selftests/kvm/lib/perf_test_util.c | 2 +-
.../kvm/memslot_modification_stress_test.c | 18 +++++++++++-------
4 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index 84982eb02b29..5d9b35d09251 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -303,7 +303,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm);
unsigned int vm_get_page_size(struct kvm_vm *vm);
unsigned int vm_get_page_shift(struct kvm_vm *vm);
-unsigned int vm_get_max_gfn(struct kvm_vm *vm);
+uint64_t vm_get_max_gfn(struct kvm_vm *vm);
int vm_get_fd(struct kvm_vm *vm);
unsigned int vm_calc_num_guest_pages(enum vm_guest_mode mode, size_t size);
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 1af1009254c4..aeffbb1e7c7d 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -2058,7 +2058,7 @@ unsigned int vm_get_page_shift(struct kvm_vm *vm)
return vm->page_shift;
}
-unsigned int vm_get_max_gfn(struct kvm_vm *vm)
+uint64_t vm_get_max_gfn(struct kvm_vm *vm)
{
return vm->max_gfn;
}
diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c
index 81490b9b4e32..ed4424ed26d6 100644
--- a/tools/testing/selftests/kvm/lib/perf_test_util.c
+++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
@@ -80,7 +80,7 @@ struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int vcpus,
*/
TEST_ASSERT(guest_num_pages < vm_get_max_gfn(vm),
"Requested more guest memory than address space allows.\n"
- " guest pages: %lx max gfn: %x vcpus: %d wss: %lx]\n",
+ " guest pages: %lx max gfn: %lx vcpus: %d wss: %lx]\n",
guest_num_pages, vm_get_max_gfn(vm), vcpus,
vcpu_memory_bytes);
diff --git a/tools/testing/selftests/kvm/memslot_modification_stress_test.c b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
index 6096bf0a5b34..98351ba0933c 100644
--- a/tools/testing/selftests/kvm/memslot_modification_stress_test.c
+++ b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
@@ -71,14 +71,22 @@ struct memslot_antagonist_args {
};
static void add_remove_memslot(struct kvm_vm *vm, useconds_t delay,
- uint64_t nr_modifications, uint64_t gpa)
+ uint64_t nr_modifications)
{
+ const uint64_t pages = 1;
+ uint64_t gpa;
int i;
+ /*
+ * Add the dummy memslot just below the perf_test_util memslot, which is
+ * at the top of the guest physical address space.
+ */
+ gpa = guest_test_phys_mem - pages * vm_get_page_size(vm);
+
for (i = 0; i < nr_modifications; i++) {
usleep(delay);
vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, gpa,
- DUMMY_MEMSLOT_INDEX, 1, 0);
+ DUMMY_MEMSLOT_INDEX, pages, 0);
vm_mem_region_delete(vm, DUMMY_MEMSLOT_INDEX);
}
@@ -120,11 +128,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
pr_info("Started all vCPUs\n");
add_remove_memslot(vm, p->memslot_modification_delay,
- p->nr_memslot_modifications,
- guest_test_phys_mem +
- (guest_percpu_mem_size * nr_vcpus) +
- perf_test_args.host_page_size +
- perf_test_args.guest_page_size);
+ p->nr_memslot_modifications);
run_vcpus = false;
--
2.31.1.818.g46aad6cb9e-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] KVM: selftests: Fix 32-bit truncation of vm_get_max_gfn()
2021-05-20 21:26 [PATCH] KVM: selftests: Fix 32-bit truncation of vm_get_max_gfn() David Matlack
@ 2021-05-20 21:47 ` Peter Xu
2021-05-20 21:56 ` David Matlack
2021-05-20 23:41 ` Venkatesh Srinivas
1 sibling, 1 reply; 5+ messages in thread
From: Peter Xu @ 2021-05-20 21:47 UTC (permalink / raw)
To: David Matlack; +Cc: kvm, Paolo Bonzini, Andrew Jones, Ben Gardon
On Thu, May 20, 2021 at 09:26:54PM +0000, David Matlack wrote:
> vm_get_max_gfn() casts vm->max_gfn from a uint64_t to an unsigned int,
> which causes the upper 32-bits of the max_gfn to get truncated.
>
> Nobody noticed until now likely because vm_get_max_gfn() is only used
> as a mechanism to create a memslot in an unused region of the guest
> physical address space (the top), and the top of the 32-bit physical
> address space was always good enough.
s/top/bottom/?
Looks right.. thanks for fixing it!
>
> This fix reveals a bug in memslot_modification_stress_test which was
> trying to create a dummy memslot past the end of guest physical memory.
> Fix that by moving the dummy memslot lower.
Would it be better to split the different fixes?
>
> Fixes: 52200d0d944e ("KVM: selftests: Remove duplicate guest mode handling")
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
> tools/testing/selftests/kvm/include/kvm_util.h | 2 +-
> tools/testing/selftests/kvm/lib/kvm_util.c | 2 +-
> .../testing/selftests/kvm/lib/perf_test_util.c | 2 +-
> .../kvm/memslot_modification_stress_test.c | 18 +++++++++++-------
> 4 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index 84982eb02b29..5d9b35d09251 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -303,7 +303,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm);
>
> unsigned int vm_get_page_size(struct kvm_vm *vm);
> unsigned int vm_get_page_shift(struct kvm_vm *vm);
> -unsigned int vm_get_max_gfn(struct kvm_vm *vm);
> +uint64_t vm_get_max_gfn(struct kvm_vm *vm);
> int vm_get_fd(struct kvm_vm *vm);
>
> unsigned int vm_calc_num_guest_pages(enum vm_guest_mode mode, size_t size);
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 1af1009254c4..aeffbb1e7c7d 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -2058,7 +2058,7 @@ unsigned int vm_get_page_shift(struct kvm_vm *vm)
> return vm->page_shift;
> }
>
> -unsigned int vm_get_max_gfn(struct kvm_vm *vm)
> +uint64_t vm_get_max_gfn(struct kvm_vm *vm)
> {
> return vm->max_gfn;
> }
> diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c
> index 81490b9b4e32..ed4424ed26d6 100644
> --- a/tools/testing/selftests/kvm/lib/perf_test_util.c
> +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
> @@ -80,7 +80,7 @@ struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int vcpus,
> */
> TEST_ASSERT(guest_num_pages < vm_get_max_gfn(vm),
> "Requested more guest memory than address space allows.\n"
> - " guest pages: %lx max gfn: %x vcpus: %d wss: %lx]\n",
> + " guest pages: %lx max gfn: %lx vcpus: %d wss: %lx]\n",
If to fix it, maybe start to use PRIu64 (and include inttypes.h)?
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] KVM: selftests: Fix 32-bit truncation of vm_get_max_gfn()
2021-05-20 21:47 ` Peter Xu
@ 2021-05-20 21:56 ` David Matlack
2021-05-20 22:46 ` Peter Xu
0 siblings, 1 reply; 5+ messages in thread
From: David Matlack @ 2021-05-20 21:56 UTC (permalink / raw)
To: Peter Xu; +Cc: kvm list, Paolo Bonzini, Andrew Jones, Ben Gardon
On Thu, May 20, 2021 at 2:47 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, May 20, 2021 at 09:26:54PM +0000, David Matlack wrote:
> > vm_get_max_gfn() casts vm->max_gfn from a uint64_t to an unsigned int,
> > which causes the upper 32-bits of the max_gfn to get truncated.
> >
> > Nobody noticed until now likely because vm_get_max_gfn() is only used
> > as a mechanism to create a memslot in an unused region of the guest
> > physical address space (the top), and the top of the 32-bit physical
> > address space was always good enough.
>
> s/top/bottom/?
I guess it depends on your reference point :). The existing comments
under tools/testing/selftests/kvm use the convention that "top" ==
high addresses.
>
> Looks right.. thanks for fixing it!
>
> >
> > This fix reveals a bug in memslot_modification_stress_test which was
> > trying to create a dummy memslot past the end of guest physical memory.
> > Fix that by moving the dummy memslot lower.
>
> Would it be better to split the different fixes?
I'm fine either way. I figured the net delta was small enough and the
fixes tightly coupled so sending as one patch made the most sense. Is
there value in splitting this up?
>
> >
> > Fixes: 52200d0d944e ("KVM: selftests: Remove duplicate guest mode handling")
> > Signed-off-by: David Matlack <dmatlack@google.com>
> > ---
> > tools/testing/selftests/kvm/include/kvm_util.h | 2 +-
> > tools/testing/selftests/kvm/lib/kvm_util.c | 2 +-
> > .../testing/selftests/kvm/lib/perf_test_util.c | 2 +-
> > .../kvm/memslot_modification_stress_test.c | 18 +++++++++++-------
> > 4 files changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> > index 84982eb02b29..5d9b35d09251 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> > @@ -303,7 +303,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm);
> >
> > unsigned int vm_get_page_size(struct kvm_vm *vm);
> > unsigned int vm_get_page_shift(struct kvm_vm *vm);
> > -unsigned int vm_get_max_gfn(struct kvm_vm *vm);
> > +uint64_t vm_get_max_gfn(struct kvm_vm *vm);
> > int vm_get_fd(struct kvm_vm *vm);
> >
> > unsigned int vm_calc_num_guest_pages(enum vm_guest_mode mode, size_t size);
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index 1af1009254c4..aeffbb1e7c7d 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -2058,7 +2058,7 @@ unsigned int vm_get_page_shift(struct kvm_vm *vm)
> > return vm->page_shift;
> > }
> >
> > -unsigned int vm_get_max_gfn(struct kvm_vm *vm)
> > +uint64_t vm_get_max_gfn(struct kvm_vm *vm)
> > {
> > return vm->max_gfn;
> > }
> > diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c
> > index 81490b9b4e32..ed4424ed26d6 100644
> > --- a/tools/testing/selftests/kvm/lib/perf_test_util.c
> > +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
> > @@ -80,7 +80,7 @@ struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int vcpus,
> > */
> > TEST_ASSERT(guest_num_pages < vm_get_max_gfn(vm),
> > "Requested more guest memory than address space allows.\n"
> > - " guest pages: %lx max gfn: %x vcpus: %d wss: %lx]\n",
> > + " guest pages: %lx max gfn: %lx vcpus: %d wss: %lx]\n",
>
> If to fix it, maybe start to use PRIu64 (and include inttypes.h)?
Will do.
Thanks!
>
> Thanks,
>
> --
> Peter Xu
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] KVM: selftests: Fix 32-bit truncation of vm_get_max_gfn()
2021-05-20 21:56 ` David Matlack
@ 2021-05-20 22:46 ` Peter Xu
0 siblings, 0 replies; 5+ messages in thread
From: Peter Xu @ 2021-05-20 22:46 UTC (permalink / raw)
To: David Matlack; +Cc: kvm list, Paolo Bonzini, Andrew Jones, Ben Gardon
On Thu, May 20, 2021 at 02:56:41PM -0700, David Matlack wrote:
> On Thu, May 20, 2021 at 2:47 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Thu, May 20, 2021 at 09:26:54PM +0000, David Matlack wrote:
> > > vm_get_max_gfn() casts vm->max_gfn from a uint64_t to an unsigned int,
> > > which causes the upper 32-bits of the max_gfn to get truncated.
> > >
> > > Nobody noticed until now likely because vm_get_max_gfn() is only used
> > > as a mechanism to create a memslot in an unused region of the guest
> > > physical address space (the top), and the top of the 32-bit physical
> > > address space was always good enough.
> >
> > s/top/bottom/?
>
> I guess it depends on your reference point :). The existing comments
> under tools/testing/selftests/kvm use the convention that "top" ==
> high addresses.
Ah I was thinking in another way (converting the unsigned int to u64 is taking
the "bottom" of the 8bytes field), while you meant we allocate memory from the
top. Yeah then it looks good.
>
> >
> > Looks right.. thanks for fixing it!
> >
> > >
> > > This fix reveals a bug in memslot_modification_stress_test which was
> > > trying to create a dummy memslot past the end of guest physical memory.
> > > Fix that by moving the dummy memslot lower.
> >
> > Would it be better to split the different fixes?
>
> I'm fine either way. I figured the net delta was small enough and the
> fixes tightly coupled so sending as one patch made the most sense. Is
> there value in splitting this up?
Feel free to keep it a single patch if no one else complains. :)
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: selftests: Fix 32-bit truncation of vm_get_max_gfn()
2021-05-20 21:26 [PATCH] KVM: selftests: Fix 32-bit truncation of vm_get_max_gfn() David Matlack
2021-05-20 21:47 ` Peter Xu
@ 2021-05-20 23:41 ` Venkatesh Srinivas
1 sibling, 0 replies; 5+ messages in thread
From: Venkatesh Srinivas @ 2021-05-20 23:41 UTC (permalink / raw)
To: David Matlack; +Cc: kvm, Paolo Bonzini, Andrew Jones, Ben Gardon, Peter Xu
On Thu, May 20, 2021 at 2:27 PM David Matlack <dmatlack@google.com> wrote:
>
> vm_get_max_gfn() casts vm->max_gfn from a uint64_t to an unsigned int,
> which causes the upper 32-bits of the max_gfn to get truncated.
>
> Nobody noticed until now likely because vm_get_max_gfn() is only used
> as a mechanism to create a memslot in an unused region of the guest
> physical address space (the top), and the top of the 32-bit physical
> address space was always good enough.
>
> This fix reveals a bug in memslot_modification_stress_test which was
> trying to create a dummy memslot past the end of guest physical memory.
> Fix that by moving the dummy memslot lower.
>
> Fixes: 52200d0d944e ("KVM: selftests: Remove duplicate guest mode handling")
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
> tools/testing/selftests/kvm/include/kvm_util.h | 2 +-
> tools/testing/selftests/kvm/lib/kvm_util.c | 2 +-
> .../testing/selftests/kvm/lib/perf_test_util.c | 2 +-
> .../kvm/memslot_modification_stress_test.c | 18 +++++++++++-------
> 4 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index 84982eb02b29..5d9b35d09251 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -303,7 +303,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm);
>
> unsigned int vm_get_page_size(struct kvm_vm *vm);
> unsigned int vm_get_page_shift(struct kvm_vm *vm);
> -unsigned int vm_get_max_gfn(struct kvm_vm *vm);
> +uint64_t vm_get_max_gfn(struct kvm_vm *vm);
> int vm_get_fd(struct kvm_vm *vm);
>
> unsigned int vm_calc_num_guest_pages(enum vm_guest_mode mode, size_t size);
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 1af1009254c4..aeffbb1e7c7d 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -2058,7 +2058,7 @@ unsigned int vm_get_page_shift(struct kvm_vm *vm)
> return vm->page_shift;
> }
>
> -unsigned int vm_get_max_gfn(struct kvm_vm *vm)
> +uint64_t vm_get_max_gfn(struct kvm_vm *vm)
> {
> return vm->max_gfn;
> }
> diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c
> index 81490b9b4e32..ed4424ed26d6 100644
> --- a/tools/testing/selftests/kvm/lib/perf_test_util.c
> +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
> @@ -80,7 +80,7 @@ struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int vcpus,
> */
> TEST_ASSERT(guest_num_pages < vm_get_max_gfn(vm),
> "Requested more guest memory than address space allows.\n"
> - " guest pages: %lx max gfn: %x vcpus: %d wss: %lx]\n",
> + " guest pages: %lx max gfn: %lx vcpus: %d wss: %lx]\n",
> guest_num_pages, vm_get_max_gfn(vm), vcpus,
> vcpu_memory_bytes);
>
> diff --git a/tools/testing/selftests/kvm/memslot_modification_stress_test.c b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> index 6096bf0a5b34..98351ba0933c 100644
> --- a/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> +++ b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> @@ -71,14 +71,22 @@ struct memslot_antagonist_args {
> };
>
> static void add_remove_memslot(struct kvm_vm *vm, useconds_t delay,
> - uint64_t nr_modifications, uint64_t gpa)
> + uint64_t nr_modifications)
> {
> + const uint64_t pages = 1;
> + uint64_t gpa;
> int i;
>
> + /*
> + * Add the dummy memslot just below the perf_test_util memslot, which is
> + * at the top of the guest physical address space.
> + */
> + gpa = guest_test_phys_mem - pages * vm_get_page_size(vm);
> +
> for (i = 0; i < nr_modifications; i++) {
> usleep(delay);
> vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, gpa,
> - DUMMY_MEMSLOT_INDEX, 1, 0);
> + DUMMY_MEMSLOT_INDEX, pages, 0);
>
> vm_mem_region_delete(vm, DUMMY_MEMSLOT_INDEX);
> }
> @@ -120,11 +128,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> pr_info("Started all vCPUs\n");
>
> add_remove_memslot(vm, p->memslot_modification_delay,
> - p->nr_memslot_modifications,
> - guest_test_phys_mem +
> - (guest_percpu_mem_size * nr_vcpus) +
> - perf_test_args.host_page_size +
> - perf_test_args.guest_page_size);
> + p->nr_memslot_modifications);
>
> run_vcpus = false;
>
> --
> 2.31.1.818.g46aad6cb9e-goog
Hah, good catch!
Reviewed-by: Venkatesh Srinivas <venkateshs@chromium.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-05-20 23:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-20 21:26 [PATCH] KVM: selftests: Fix 32-bit truncation of vm_get_max_gfn() David Matlack
2021-05-20 21:47 ` Peter Xu
2021-05-20 21:56 ` David Matlack
2021-05-20 22:46 ` Peter Xu
2021-05-20 23:41 ` Venkatesh Srinivas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.