Re: [PATCH 2/2] KVM: selftests: Test guest_memfd binding overlap without GPA overlap

[Ackerley Tng]

ZongYao.Chen@xxxxxxxxxxxxxxxxx writes:

> From: Zongyao Chen <ZongYao.Chen@xxxxxxxxxxxxxxxxx>
>
> 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 <ZongYao.Chen@xxxxxxxxxxxxxxxxx>
> ---
>  .../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 <ackerleytng@xxxxxxxxxx>
Tested-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>

> --
> 2.47.3