| Message ID | 20210624070931.565-1-yuzenghui@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: Fix mapping length truncation in m{,un}map() | expand |
On 24/06/21 09:09, Zenghui Yu wrote: > max_mem_slots is now declared as uint32_t. The result of (0x200000 * 32767) > is unexpectedly truncated to be 0xffe00000, whilst we actually need to > allocate about, 63GB. Cast max_mem_slots to size_t in both mmap() and > munmap() to fix the length truncation. > > We'll otherwise see the failure on arm64 thanks to the access_ok() checking > in __kvm_set_memory_region(), as the unmapped VA happen to go beyond the > task's allowed address space. > > # ./set_memory_region_test > Allowed number of memory slots: 32767 > Adding slots 0..32766, each memory region with 2048K size > ==== Test Assertion Failure ==== > set_memory_region_test.c:391: ret == 0 > pid=94861 tid=94861 errno=22 - Invalid argument > 1 0x00000000004015a7: test_add_max_memory_regions at set_memory_region_test.c:389 > 2 (inlined by) main at set_memory_region_test.c:426 > 3 0x0000ffffb8e67bdf: ?? ??:0 > 4 0x00000000004016db: _start at :? > KVM_SET_USER_MEMORY_REGION IOCTL failed, > rc: -1 errno: 22 slot: 2615 > > Fixes: 3bf0fcd75434 ("KVM: selftests: Speed up set_memory_region_test") > Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> > --- > tools/testing/selftests/kvm/set_memory_region_test.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c > index d79d58eada9f..85b18bb8f762 100644 > --- a/tools/testing/selftests/kvm/set_memory_region_test.c > +++ b/tools/testing/selftests/kvm/set_memory_region_test.c > @@ -376,7 +376,7 @@ static void test_add_max_memory_regions(void) > pr_info("Adding slots 0..%i, each memory region with %dK size\n", > (max_mem_slots - 1), MEM_REGION_SIZE >> 10); > > - mem = mmap(NULL, MEM_REGION_SIZE * max_mem_slots + alignment, > + mem = mmap(NULL, (size_t)max_mem_slots * MEM_REGION_SIZE + alignment, > PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > TEST_ASSERT(mem != MAP_FAILED, "Failed to mmap() host"); > mem_aligned = (void *)(((size_t) mem + alignment - 1) & ~(alignment - 1)); > @@ -401,7 +401,7 @@ static void test_add_max_memory_regions(void) > TEST_ASSERT(ret == -1 && errno == EINVAL, > "Adding one more memory slot should fail with EINVAL"); > > - munmap(mem, MEM_REGION_SIZE * max_mem_slots + alignment); > + munmap(mem, (size_t)max_mem_slots * MEM_REGION_SIZE + alignment); > munmap(mem_extra, MEM_REGION_SIZE); > kvm_vm_free(vm); > } > Queued, thanks. Paolo
On 24.06.21 09:09, Zenghui Yu wrote: > max_mem_slots is now declared as uint32_t. The result of (0x200000 * 32767) > is unexpectedly truncated to be 0xffe00000, whilst we actually need to > allocate about, 63GB. Cast max_mem_slots to size_t in both mmap() and > munmap() to fix the length truncation. > > We'll otherwise see the failure on arm64 thanks to the access_ok() checking > in __kvm_set_memory_region(), as the unmapped VA happen to go beyond the > task's allowed address space. > > # ./set_memory_region_test > Allowed number of memory slots: 32767 > Adding slots 0..32766, each memory region with 2048K size > ==== Test Assertion Failure ==== > set_memory_region_test.c:391: ret == 0 > pid=94861 tid=94861 errno=22 - Invalid argument > 1 0x00000000004015a7: test_add_max_memory_regions at set_memory_region_test.c:389 > 2 (inlined by) main at set_memory_region_test.c:426 > 3 0x0000ffffb8e67bdf: ?? ??:0 > 4 0x00000000004016db: _start at :? > KVM_SET_USER_MEMORY_REGION IOCTL failed, > rc: -1 errno: 22 slot: 2615 > > Fixes: 3bf0fcd75434 ("KVM: selftests: Speed up set_memory_region_test") > Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> While likely correct, this now breaks on many test systems in our CI that have less memory than 64GB. (We do get ENOMEM). I have not seen the ENOMEM failures in earlier versions. Strange > --- > tools/testing/selftests/kvm/set_memory_region_test.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c > index d79d58eada9f..85b18bb8f762 100644 > --- a/tools/testing/selftests/kvm/set_memory_region_test.c > +++ b/tools/testing/selftests/kvm/set_memory_region_test.c > @@ -376,7 +376,7 @@ static void test_add_max_memory_regions(void) > pr_info("Adding slots 0..%i, each memory region with %dK size\n", > (max_mem_slots - 1), MEM_REGION_SIZE >> 10); > > - mem = mmap(NULL, MEM_REGION_SIZE * max_mem_slots + alignment, > + mem = mmap(NULL, (size_t)max_mem_slots * MEM_REGION_SIZE + alignment, > PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > TEST_ASSERT(mem != MAP_FAILED, "Failed to mmap() host"); > mem_aligned = (void *)(((size_t) mem + alignment - 1) & ~(alignment - 1)); > @@ -401,7 +401,7 @@ static void test_add_max_memory_regions(void) > TEST_ASSERT(ret == -1 && errno == EINVAL, > "Adding one more memory slot should fail with EINVAL"); > > - munmap(mem, MEM_REGION_SIZE * max_mem_slots + alignment); > + munmap(mem, (size_t)max_mem_slots * MEM_REGION_SIZE + alignment); > munmap(mem_extra, MEM_REGION_SIZE); > kvm_vm_free(vm); > } >
On 30.06.21 09:37, Christian Borntraeger wrote: > > > On 24.06.21 09:09, Zenghui Yu wrote: >> max_mem_slots is now declared as uint32_t. The result of (0x200000 * 32767) >> is unexpectedly truncated to be 0xffe00000, whilst we actually need to >> allocate about, 63GB. Cast max_mem_slots to size_t in both mmap() and >> munmap() to fix the length truncation. >> >> We'll otherwise see the failure on arm64 thanks to the access_ok() checking >> in __kvm_set_memory_region(), as the unmapped VA happen to go beyond the >> task's allowed address space. >> >> # ./set_memory_region_test >> Allowed number of memory slots: 32767 >> Adding slots 0..32766, each memory region with 2048K size >> ==== Test Assertion Failure ==== >> set_memory_region_test.c:391: ret == 0 >> pid=94861 tid=94861 errno=22 - Invalid argument >> 1 0x00000000004015a7: test_add_max_memory_regions at set_memory_region_test.c:389 >> 2 (inlined by) main at set_memory_region_test.c:426 >> 3 0x0000ffffb8e67bdf: ?? ??:0 >> 4 0x00000000004016db: _start at :? >> KVM_SET_USER_MEMORY_REGION IOCTL failed, >> rc: -1 errno: 22 slot: 2615 >> >> Fixes: 3bf0fcd75434 ("KVM: selftests: Speed up set_memory_region_test") >> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> > > While likely correct, this now breaks on many test systems in our CI that have less memory than 64GB. > (We do get ENOMEM). I have not seen the ENOMEM failures in earlier versions. Strange As we do not need the memory, the following seems to do the trick (MAP_NORESERVE) diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c index d8812f27648c..0fc68371fb1b 100644 --- a/tools/testing/selftests/kvm/set_memory_region_test.c +++ b/tools/testing/selftests/kvm/set_memory_region_test.c @@ -377,7 +377,7 @@ static void test_add_max_memory_regions(void) (max_mem_slots - 1), MEM_REGION_SIZE >> 10); mem = mmap(NULL, (size_t)max_mem_slots * MEM_REGION_SIZE + alignment, - PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0); TEST_ASSERT(mem != MAP_FAILED, "Failed to mmap() host"); mem_aligned = (void *)(((size_t) mem + alignment - 1) & ~(alignment - 1));
diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c index d79d58eada9f..85b18bb8f762 100644 --- a/tools/testing/selftests/kvm/set_memory_region_test.c +++ b/tools/testing/selftests/kvm/set_memory_region_test.c @@ -376,7 +376,7 @@ static void test_add_max_memory_regions(void) pr_info("Adding slots 0..%i, each memory region with %dK size\n", (max_mem_slots - 1), MEM_REGION_SIZE >> 10); - mem = mmap(NULL, MEM_REGION_SIZE * max_mem_slots + alignment, + mem = mmap(NULL, (size_t)max_mem_slots * MEM_REGION_SIZE + alignment, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); TEST_ASSERT(mem != MAP_FAILED, "Failed to mmap() host"); mem_aligned = (void *)(((size_t) mem + alignment - 1) & ~(alignment - 1)); @@ -401,7 +401,7 @@ static void test_add_max_memory_regions(void) TEST_ASSERT(ret == -1 && errno == EINVAL, "Adding one more memory slot should fail with EINVAL"); - munmap(mem, MEM_REGION_SIZE * max_mem_slots + alignment); + munmap(mem, (size_t)max_mem_slots * MEM_REGION_SIZE + alignment); munmap(mem_extra, MEM_REGION_SIZE); kvm_vm_free(vm); }
max_mem_slots is now declared as uint32_t. The result of (0x200000 * 32767) is unexpectedly truncated to be 0xffe00000, whilst we actually need to allocate about, 63GB. Cast max_mem_slots to size_t in both mmap() and munmap() to fix the length truncation. We'll otherwise see the failure on arm64 thanks to the access_ok() checking in __kvm_set_memory_region(), as the unmapped VA happen to go beyond the task's allowed address space. # ./set_memory_region_test Allowed number of memory slots: 32767 Adding slots 0..32766, each memory region with 2048K size ==== Test Assertion Failure ==== set_memory_region_test.c:391: ret == 0 pid=94861 tid=94861 errno=22 - Invalid argument 1 0x00000000004015a7: test_add_max_memory_regions at set_memory_region_test.c:389 2 (inlined by) main at set_memory_region_test.c:426 3 0x0000ffffb8e67bdf: ?? ??:0 4 0x00000000004016db: _start at :? KVM_SET_USER_MEMORY_REGION IOCTL failed, rc: -1 errno: 22 slot: 2615 Fixes: 3bf0fcd75434 ("KVM: selftests: Speed up set_memory_region_test") Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> --- tools/testing/selftests/kvm/set_memory_region_test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)