2021-06-24 07:11:39

by Zenghui Yu

Subject: [PATCH] KVM: selftests: Fix mapping length truncation in m{,un}map()

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 <[email protected]>
---
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);
}
--
2.19.1

2021-06-24 07:21:04

by Paolo Bonzini

Subject: Re: [PATCH] KVM: selftests: Fix mapping length truncation in m{,un}map()

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 <[email protected]>
> ---
> 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

2021-06-30 07:39:32

by Christian Borntraeger

Subject: Re: [PATCH] KVM: selftests: Fix mapping length truncation in m{,un}map()

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 <[email protected]>

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);
> }
>

2021-06-30 08:27:37

by Christian Borntraeger

Subject: Re: [PATCH] KVM: selftests: Fix mapping length truncation in m{,un}map()

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 <[email protected]>
>
> 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));