Re: [PATCH RFC 11/17] KVM: selftests: Test guest_memfd with anonymous fsmount() tmpfs pool

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] kselftests: kvm: incorrect fd validation in create_tmpfs_pool_fd()
- [Medium] kselftests: kvm: brittle invalid size test implicitly uses resource_fd=0
- [Medium] kselftests: kvm: test fails on older kernels due to missing flag cap check
--

commit b9ccbfbf6c1bd14da922a66c6dd6f5735cd34ba0
Author: Ackerley Tng <ackerleytng@google.com>

KVM: selftests: Test guest_memfd with anonymous fsmount() tmpfs pool

Add test coverage verifying that creating a guest_memfd backed by an
anonymous tmpfs mount descriptor returned by fsmount() succeeds and
reports the requested file size.

> diff --git a/tools/testing/selftests/kvm/guest_memfd_test.c b/tools/testing/selftests/kvm/guest_memfd_test.c
> index e8199ed17b0f8..73212d5307049 100644
> --- a/tools/testing/selftests/kvm/guest_memfd_test.c
> +++ b/tools/testing/selftests/kvm/guest_memfd_test.c

[ ... ]

> @@ -541,12 +541,23 @@ static void test_resource_fd_tmpfs_huge(struct kvm_vm *vm)
>  	close(pool_fd);
>  }
>  
> -#define ____gmem_test(__test, __vm, __flags, __gmem_size, args...)	\
> -do {									\
> -	int fd = vm_create_guest_memfd(__vm, __gmem_size, __flags);	\
> -									\
> -	test_##__test(args);						\
> -	close(fd);							\
> +#define ____gmem_test(__test, __vm, __flags, __gmem_size, args...)		\
> +do {										\
> +	int pool_fd = -1;							\
> +	int fd;									\
> +										\
> +	if ((__flags) & GUEST_MEMFD_FLAG_USE_RESOURCE) {			\
> +		pool_fd = create_tmpfs_pool_fd("never", true, __gmem_size);	\

[Severity: Medium]
Does create_tmpfs_pool_fd() correctly handle when fsmount() returns 0?

Looking at create_tmpfs_pool_fd():

    mnt_fd = syscall(__NR_fsmount, fs_fd, FSMOUNT_CLOEXEC, 0);
    TEST_ASSERT(mnt_fd > 0, "fsmount failed");

While typically stdin, file descriptor 0 is valid. If fd 0 is allocated to the
new mount, a successful fsmount will trigger a false test failure because the
assertion strictly checks for mnt_fd > 0 instead of mnt_fd >= 0.

> +		fd = vm_create_guest_memfd_resource(__vm, __gmem_size,		\
> +						    __flags, pool_fd);		\
> +	} else {								\
> +		fd = vm_create_guest_memfd(__vm, __gmem_size, __flags);	\
> +	}									\

[ ... ]

> @@ -606,15 +617,23 @@ static void test_guest_memfd(unsigned long vm_type)
>  	test_resource_fd_tmpfs_huge(vm);
>  
>  	__test_guest_memfd(vm, 0);
> +	__test_guest_memfd(vm, GUEST_MEMFD_FLAG_USE_RESOURCE);

[Severity: Medium]
Does this implicitly pass resource_fd=0 when testing the USE_RESOURCE flag?

Looking at test_create_guest_memfd_invalid_sizes(), which is called via
__test_guest_memfd():

    fd = __vm_create_guest_memfd(vm, size, guest_memfd_flags);
    TEST_ASSERT(fd < 0 && errno == EINVAL, ...);

Because __vm_create_guest_memfd() defaults to 0 for the resource_fd, it relies
entirely on kernel parameter validation ordering. If the kernel's parameter
validation evaluates resource_fd before size, this will unexpectedly return
EBADF or ENOTDIR (as fd 0 is typically not a valid tmpfs mount), causing false
test failures instead of the expected EINVAL for unaligned sizes.

[Severity: Medium]
Does this unconditionally execute the USE_RESOURCE tests without checking if the
flag is supported?

On older kernels, passing an unsupported flag returns -EINVAL. This will cause
test failures since the feature is not exposed via
vm_check_cap(KVM_CAP_GUEST_MEMFD_FLAGS) and skipped gracefully. For example,
test_resource_fd_invalid(), which is called just above this line, expects
-EBADF but will receive -EINVAL when the USE_RESOURCE flag is unrecognized:

    fd = __vm_create_guest_memfd_resource(vm, page_size,
                                          GUEST_MEMFD_FLAG_USE_RESOURCE, -1);
    TEST_ASSERT(fd < 0, "guest_memfd with -1 resource_fd should fail");
    TEST_ASSERT_EQ(errno, EBADF);

>  
>  	flags = vm_check_cap(vm, KVM_CAP_GUEST_MEMFD_FLAGS);
> -	if (flags & GUEST_MEMFD_FLAG_MMAP)
> +	if (flags & GUEST_MEMFD_FLAG_MMAP) {
>  		__test_guest_memfd(vm, GUEST_MEMFD_FLAG_MMAP);
> +		__test_guest_memfd(vm, GUEST_MEMFD_FLAG_MMAP |
> +				       GUEST_MEMFD_FLAG_USE_RESOURCE);
> +	}
>  
>  	/* MMAP should always be supported if INIT_SHARED is supported. */
> -	if (flags & GUEST_MEMFD_FLAG_INIT_SHARED)
> +	if (flags & GUEST_MEMFD_FLAG_INIT_SHARED) {
>  		__test_guest_memfd(vm, GUEST_MEMFD_FLAG_MMAP |
>  				       GUEST_MEMFD_FLAG_INIT_SHARED);
> +		__test_guest_memfd(vm, GUEST_MEMFD_FLAG_MMAP |
> +				       GUEST_MEMFD_FLAG_INIT_SHARED |
> +				       GUEST_MEMFD_FLAG_USE_RESOURCE);
> +	}
>  
>  	kvm_vm_free(vm);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925-gmem-tmpfs-backend-v1-0-d36159822d18@google.com?part=11