Re: [PATCH] KVM: selftests: Add tests - invalid inputs for KVM_CREATE_GUEST_MEMFD

[Sean Christopherson]

On Mon, Aug 21, 2023, Ackerley Tng wrote:
> Test that invalid inputs for KVM_CREATE_GUEST_MEMFD, such as
> non-page-aligned page size and invalid flags, are rejected by the
> KVM_CREATE_GUEST_MEMFD with EINVAL
> 
> Signed-off-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>
> ---
>  .../testing/selftests/kvm/guest_memfd_test.c  | 33 +++++++++++++++++++
>  .../selftests/kvm/include/kvm_util_base.h     | 11 +++++--
>  2 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/guest_memfd_test.c b/tools/testing/selftests/kvm/guest_memfd_test.c
> index eb93c608a7e0..a8e37f001297 100644
> --- a/tools/testing/selftests/kvm/guest_memfd_test.c
> +++ b/tools/testing/selftests/kvm/guest_memfd_test.c
> @@ -90,6 +90,37 @@ static void test_fallocate(int fd, size_t page_size, size_t total_size)
>  	TEST_ASSERT(!ret, "fallocate to restore punched hole should succeed");
>  }
>  
> +static void test_create_guest_memfd_invalid(struct kvm_vm *vm, size_t page_size)
> +{
> +	int fd;
> +	uint64_t size;

This should be size_t.

> +	uint64_t flags;
> +	uint64_t valid_flags = 0;

Revert fir/xmas-tree please.

> +
> +	for (size = 1; size < page_size; size++) {
> +		fd = __vm_create_guest_memfd(vm, size, 0);
> +		TEST_ASSERT(

No, bad Google3, bad.  Never immediately wrap after an opening parenthesis.

> +			fd == -1,
> +			"Creating guest memfds with non-page-aligned page sizes should fail");
> +		TEST_ASSERT(errno == EINVAL, "... and errno should be set to EINVAL");

Don't split/delay "errno" checks, it's all too easy for errno to get clobbered.
And there's absolutely zero reason to split these, the ret+errno get printed so
the odds of what went wrong not being super duper obvious are very low.  What
_is_ worth printing is the size.

> +	}
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +	valid_flags = KVM_GUEST_MEMFD_ALLOW_HUGEPAGE;
> +#endif

Ugh, this is annoying.  But HPAGE_PMD_SIZE wrapping with CONFIG_TRANSPARENT_HUGEPAGE
and so guest_memfd() can't (easily) enforce the alignment check if THP is disabled,
i.e. always letting userspace specify KVM_GUEST_MEMFD_ALLOW_HUGEPAGE would be
messy.

Oh!  And we should also test for unaligned huge pages, i.e. multiples of page_size
that aren't PMD-aligned.  At that point, I would say don't pass in @page_size to
this particular testcase, e.g. have main() be something like this:

	vm = vm_create_barebones();

	test_create_guest_memfd_invalid(vm);

	page_size = getpagesize();
	total_size = page_size * 4;
	fd = vm_create_guest_memfd(vm, total_size, 0);

	test_file_read_write(fd);
	test_mmap(fd, page_size);
	test_file_size(fd, page_size, total_size);
	test_fallocate(fd, page_size, total_size);

And then in here, use get_trans_hugepagesz() to do negative testing of
KVM_GUEST_MEMFD_ALLOW_HUGEPAGE.

> +
> +	for (flags = 1; flags; flags <<= 1) {
> +		if (flags & valid_flags)

This only ever tests one flag in isolation, e.g. if it would detect if KVM did
something ridiculous like

	if (flags && !(flags & KVM_GUEST_MEMFD_ALLOW_HUGEPAGE))
		return -EINVAL;

Iterating over all possible values doesn't make sense, and giving "lower" flags
preference is likewise a bit silly, so what if we do (note the s/flags/flag)

	for (flag = 1; flag; flag <<= 1) {
		if (flag & valid_flags)
			continue;

		fd = __vm_create_guest_memfd(vm, page_size, flag);
		TEST_ASSERT(fd == -1 && errno == EINVAL,
			    "guest_memfd() with flags '0x%llx' should fail with EINVAL", flag);

		for_each_set_bit(bit, &valid_flags, 64) {
			fd = __vm_create_guest_memfd(vm, page_size, flag | BIT_ULL(bit));
			TEST_ASSERT(fd == -1 && errno == EINVAL,
				    "guest_memfd() with flags '0x%llx' should fail with EINVAL",
			    	    flag | BIT_ULL(bit));
		}
	}

i.e. test the invalid flag in isolation, and then also test it in combination with
each valid flag.  It's from from exhaustive, but it'll at least ensure we have *some*
coverage if/when new flags come along.