[email protected] writes: > From: Zongyao Chen <[email protected]> > > The guest_memfd binding overlap test recreates the deleted slot with GPA > ranges that overlap the still-live slot. KVM rejects those attempts from > the generic memslot overlap check before reaching kvm_gmem_bind(), so the > test can pass even if guest_memfd binding overlap detection is broken. > > Recreate the slot at its original, non-overlapping GPA and use guest_memfd > offsets that overlap the front and back halves of the other slot's binding. > Expand the guest_memfd so the back-half case remains within the file size. > > Fixes: 2feabb855df8 ("KVM: selftests: Expand set_memory_region_test to > validate guest_memfd()")
Thanks for fixing this! > Signed-off-by: Zongyao Chen <[email protected]> > --- > .../testing/selftests/kvm/set_memory_region_test.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c > b/tools/testing/selftests/kvm/set_memory_region_test.c > index 9b919a231c93..15607e0bec90 100644 > --- a/tools/testing/selftests/kvm/set_memory_region_test.c > +++ b/tools/testing/selftests/kvm/set_memory_region_test.c > @@ -510,7 +510,7 @@ static void > test_add_overlapping_private_memory_regions(void) Shall we rename this to test_bind_overlapping_guest_memfd_offsets to make it clearer? Perhaps also update the pr_info() to "Testing binding to overlapping guest_memfd offsets\n". > > vm = vm_create_barebones_type(KVM_X86_SW_PROTECTED_VM); > > - memfd = vm_create_guest_memfd(vm, MEM_REGION_SIZE * 4, 0); > + memfd = vm_create_guest_memfd(vm, MEM_REGION_SIZE * 6, 0); I think this technically only needs to be MEM_REGION_SIZE * 5 for this test to work. > > vm_set_user_memory_region2(vm, MEM_REGION_SLOT, KVM_MEM_GUEST_MEMFD, > MEM_REGION_GPA, MEM_REGION_SIZE * 2, 0, > memfd, 0); > @@ -526,19 +526,19 @@ static void > test_add_overlapping_private_memory_regions(void) > vm_set_user_memory_region2(vm, MEM_REGION_SLOT, KVM_MEM_GUEST_MEMFD, > MEM_REGION_GPA, 0, NULL, -1, 0); When I re-read this I was wondering why we created and removed the first memslot. Was it meant as a confidence check that set_memory_region works with the given MEM_REGION_GPA? Perhaps we could add a comment/pr_info() to check that. > > - /* Overlap the front half of the other slot. */ > + /* Overlap the front half of the other slot's guest_memfd binding. */ > r = __vm_set_user_memory_region2(vm, MEM_REGION_SLOT, > KVM_MEM_GUEST_MEMFD, > - MEM_REGION_GPA * 2 - MEM_REGION_SIZE, > + MEM_REGION_GPA, > MEM_REGION_SIZE * 2, > - 0, memfd, 0); > + 0, memfd, MEM_REGION_SIZE); > TEST_ASSERT(r == -1 && errno == EEXIST, "%s", > "Overlapping guest_memfd() bindings should fail with > EEXIST"); > > - /* And now the back half of the other slot. */ > + /* And now the back half of the other slot's guest_memfd binding. */ > r = __vm_set_user_memory_region2(vm, MEM_REGION_SLOT, > KVM_MEM_GUEST_MEMFD, > - MEM_REGION_GPA * 2 + MEM_REGION_SIZE, > + MEM_REGION_GPA, > MEM_REGION_SIZE * 2, > - 0, memfd, 0); > + 0, memfd, MEM_REGION_SIZE * 3); > TEST_ASSERT(r == -1 && errno == EEXIST, "%s", > "Overlapping guest_memfd() bindings should fail with > EEXIST"); > Since this test program is meant to test set_memory_region, should we be retaining the original test? The original test is wrong in that it doesn't test guest_memfd's binding, but it does test that set_memory_region returns -EEXIST on overlapping GPAs. Perhaps to just test overlapping GPAs we can use anonymous memory instead of guest_memfd. Reviewed-by: Ackerley Tng <[email protected]> Tested-by: Ackerley Tng <[email protected]> > -- > 2.47.3
