Re: [PATCH RFC v3 50/88] KVM: selftests: Test guest_memfd conversions with sample memory provider backing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Low] KVM: selftests: file descriptor leak of gup_test_fd in
  __pin_pages()
- [Low] KVM: selftests: gmem_fd leak in gmem_conversions_do_setup()
- [Low] KVM: selftests: wrong relative include path to memory_provider.h
--

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

KVM: selftests: Test guest_memfd conversions with sample memory provider backing

This commit adds a fixture variant to guest_memfd_conversions_test to test
guest_memfd conversions backed by the sample memory provider, and adds a
__pin_pages() helper so the elevated_refcount test can check that pinning
PFN-backed memory fails.

> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index d42e201eba763..9793818f54d23 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -674,7 +674,7 @@ int __pin_task_to_cpu(pthread_t task, int cpu)
>  
>  static int gup_test_fd = -1;
>  
> -void pin_pages(void *vaddr, uint64_t size)
> +int __pin_pages(void *vaddr, uint64_t size)
>  {
>  	const struct pin_longterm_test args = {
>  		.addr = (uint64_t)vaddr,
> @@ -685,7 +685,12 @@ void pin_pages(void *vaddr, uint64_t size)
>  	gup_test_fd = __open_path_or_exit("/sys/kernel/debug/gup_test", O_RDWR,
>  					  "Is CONFIG_GUP_TEST enabled?");
>  
> -	TEST_ASSERT_EQ(ioctl(gup_test_fd, PIN_LONGTERM_TEST_START, &args), 0);
> +	return ioctl(gup_test_fd, PIN_LONGTERM_TEST_START, &args);
> +}

[Severity: Low]
Does __pin_pages() leak gup_test_fd when the PIN_LONGTERM_TEST_START ioctl
fails?  In the elevated_refcount multipage test for the mp variant,
__pin_pages() is expected to fail and the caller returns early without
calling unpin_pages(), so each loop iteration overwrites gup_test_fd
without closing the previous descriptor.  Closing and resetting
gup_test_fd via kvm_free_fd(gup_test_fd) when the ioctl returns an error
would avoid the leak.

> diff --git a/tools/testing/selftests/kvm/x86/guest_memfd_conversions_test.c b/tools/testing/selftests/kvm/x86/guest_memfd_conversions_test.c
> index 023df9cb9d4a6..d0cbb1353d7ee 100644
> --- a/tools/testing/selftests/kvm/x86/guest_memfd_conversions_test.c
> +++ b/tools/testing/selftests/kvm/x86/guest_memfd_conversions_test.c
> @@ -16,11 +16,13 @@
>  #include "kselftest_harness.h"
>  #include "test_util.h"
>  #include "ucall_common.h"
> +#include "../../../../samples/kvm/memory_provider.h"

[Severity: Low]
Should this relative path have five "../" components instead of four?
Files under tools/testing/selftests/kvm/x86/ sit five levels below the
repository root (like tools/testing/selftests/kvm/lib/kvm_util.c, which
includes "../../../../../mm/gup_test.h").  With four "../" components, the
preprocessor searches two levels above the kernel tree via
-I$(LINUX_TOOL_INCLUDE) before resolving the header through
-I$(LINUX_TOOL_ARCH_INCLUDE), which breaks file-relative lookup for tools
and risks picking up a file outside the checkout.

[ ... ]

> @@ -75,10 +109,12 @@ static void gmem_conversions_do_setup(test_data_t *t,
>  
>  	vm = __vm_create_shape_with_one_vcpu(shape, &t->vcpu, nr_pages, guest_do_rmw);
>  
> +	t->expect_pin_failure = variant->expect_pin_failure;
>  	t->resource_fd = -1;
>  
>  	if (variant->create_resource) {
> -		uint64_t flags = gmem_flags | GUEST_MEMFD_FLAG_USE_RESOURCE;
> +		uint64_t flags = gmem_flags | GUEST_MEMFD_FLAG_USE_RESOURCE |
> +				 variant->extra_flags;
>  
>  		t->resource_fd = variant->create_resource(size);
>  		TEST_ASSERT(t->resource_fd >= 0, "Failed to create backing resource");

[Severity: Low]
Does gmem_conversions_do_setup() leak the local gmem_fd created by
__vm_create_guest_memfd_resource() for resource-backed variants?
vm_mem_add() duplicates a caller-supplied gmem_fd via kvm_dup(gmem_fd)
rather than taking ownership of it:

tools/testing/selftests/kvm/x86/guest_memfd_conversions_test.c:gmem_conversions_do_setup() {
	if (variant->create_resource) {
		...
		gmem_fd = __vm_create_guest_memfd_resource(vm, size, flags,
							   t->resource_fd);
		TEST_ASSERT(gmem_fd >= 0, "Failed to create guest_memfd with resource");
	}

	vm_mem_add(vm, VM_MEM_SRC_SHMEM, gpa, slot, nr_pages,
		   KVM_MEM_GUEST_MEMFD, gmem_fd, 0, gmem_flags);
}

Because gmem_fd is never closed after vm_mem_add(), the guest_memfd file
and its attached backing resource (including the CMA pages allocated by
the memory_provider module in the mp variant) stay pinned across teardown
in multipage test loops until the process exits.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-gmem-alternative-providers-v3-0-8484dde66a7e@google.com?part=50