| Message ID | 20210520212654.712276-1-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: Fix 32-bit truncation of vm_get_max_gfn() | expand |
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,
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 >
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,
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>
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;
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(-)