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.