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.
