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@google.com
.../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.