Re: [PATCH v8 38/46] KVM: selftests: Add helpers to pin pages with CONFIG_GUP_TEST

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] System-wide exclusivity of `gup_test` prevents parallel execution of KVM selftests and causes spurious failures.
- [Low] Incorrect relative include path for `gup_test.h` breaks the build or relies on accidental `-I` path traversal.

Pre-existing issues:
- [High] The `gup_test_release()` function calls `pin_longterm_test_stop()` without acquiring the `pin_longterm_test_mutex`, leading to a data race, double free, and Use-After-Free.
--

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

KVM: selftests: Add helpers to pin pages with CONFIG_GUP_TEST

This commit adds helper functions allowing KVM selftests to pin memory using
the /sys/kernel/debug/gup_test interface. It implements pin_pages() and
unpin_pages() by wrapping ioctl calls to PIN_LONGTERM_TEST_START and
PIN_LONGTERM_TEST_STOP.

> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index b73817f7bc803..524ef97d634bf 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -18,6 +18,8 @@
>  #include <unistd.h>
>  #include <linux/kernel.h>
>  
> +#include "../../../../mm/gup_test.h"

[Severity: Low]
Is this relative include path correct?

Four levels up from tools/testing/selftests/kvm/lib/ only reaches the
tools/ directory, causing the compiler to search for tools/mm/gup_test.h.

Does the build only succeed because the compiler falls back to the
-I ../rseq include path defined in the KVM selftests Makefile?

[ ... ]

> @@ -639,6 +641,27 @@ int __pin_task_to_cpu(pthread_t task, int cpu)
>  	return pthread_setaffinity_np(task, sizeof(cpuset), &cpuset);
>  }
>  
> +static int gup_test_fd = -1;
> +
> +void pin_pages(void *vaddr, uint64_t size)
> +{
> +	const struct pin_longterm_test args = {
> +		.addr = (uint64_t)vaddr,
> +		.size = size,
> +		.flags = PIN_LONGTERM_TEST_FLAG_USE_WRITE,
> +	};
> +
> +	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);

[Severity: Medium]
Will this cause spurious failures when running tests in parallel?

The /sys/kernel/debug/gup_test interface relies on system-wide global
state (pin_longterm_test_pages) rather than per-file-descriptor state.

If multiple tests run concurrently, won't the kernel return -EINVAL to all
but the first caller because the global state is already occupied?

Furthermore, when one test exits, its file descriptor closure unpins the
memory of whichever test was actively using it.

[Severity: High]
This isn't a bug introduced by this patch, but interacting with the gup_test
file descriptor exposes a data race in the kernel.

In mm/gup_test.c, closing the file descriptor calls gup_test_release(),
which invokes pin_longterm_test_stop() without holding the
pin_longterm_test_mutex:

mm/gup_test.c:gup_test_release() {
    pin_longterm_test_stop();
    return 0;
}

If multiple file descriptors are closed concurrently (e.g., multithreaded
test applications or concurrent test processes exiting), multiple threads
can race inside pin_longterm_test_stop() and concurrently call
unpin_user_pages() and kvfree() on the same global pin_longterm_test_pages
pointer, leading to a double free and use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-gmem-inplace-conversion-v8-0-9d2959357853@google.com?part=38