Re: [PATCH v2 2/2] selftests/mm: add a uffd test for re-registering during UFFDIO_COPY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/mm: poll() returns POLLERR in uffd_wp_copy_reregister_test()
--

commit 58286e73577bfa5312c30b5cdc10c8a436006bfb
Author: Jose A. Perez de Azpillaga <azpijr@gmail.com>

selftests/mm: add a uffd test for re-registering during UFFDIO_COPY

This commit adds the wp-copy-reregister selftest to verify that re-registering
a destination VMA without write-protection while UFFDIO_COPY with
UFFDIO_COPY_MODE_WP is waiting on a source page fault fails with -EINVAL and
does not leave a uffd-wp bit behind.

> diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c
> index 2a97ba1d6feb1..cc22b4cde621c 100644
> --- a/tools/testing/selftests/mm/uffd-unit-tests.c
> +++ b/tools/testing/selftests/mm/uffd-unit-tests.c
> @@ -375,6 +375,104 @@ static void uffd_wp_unpopulated_test(uffd_global_test_opts_t *gopts, uffd_test_a

[ ... ]

> +static void uffd_wp_copy_reregister_test(uffd_global_test_opts_t *gopts,
> +					 uffd_test_args_t *args)
> +{
> +	struct uffdio_api api = { .api = UFFD_API };
> +	struct wp_copy_args copy_args = { .gopts = gopts };
> +	unsigned long len = gopts->page_size;
> +	struct uffdio_zeropage zeropage;
> +	struct uffd_msg msg;
> +	struct pollfd pfd;
> +	pthread_t thread;
> +	uint64_t value;
> +	int pagemap_fd;
> +	int src_uffd;
> +
> +	/* The source fault comes from the kernel, so no UFFD_USER_MODE_ONLY */
> +	src_uffd = uffd_open(O_CLOEXEC);
> +	if (src_uffd < 0) {
> +		uffd_test_skip("cannot open a kernel-mode userfaultfd");
> +		return;
> +	}
> +	if (ioctl(src_uffd, UFFDIO_API, &api))
> +		err("UFFDIO_API failed");
> +
> +	copy_args.src = mmap(NULL, len, PROT_READ | PROT_WRITE,
> +			     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +	if (copy_args.src == MAP_FAILED)
> +		err("mmap() failed");
> +
> +	if (uffd_register(src_uffd, copy_args.src, len, true, false, false))
> +		err("register src failed");
> +	if (uffd_register(gopts->uffd, gopts->area_dst, len, true, true, false))
> +		err("register dst failed");
> +
> +	if (pthread_create(&thread, NULL, wp_copy_thread, &copy_args))
> +		err("pthread_create() failed");
> +
> +	/* Wait until UFFDIO_COPY blocks on the source page */
> +	pfd = (struct pollfd){ .fd = src_uffd, .events = POLLIN };
> +	if (poll(&pfd, 1, 10000) != 1)
> +		err("no fault on the source page");
> +	if (read(src_uffd, &msg, sizeof(msg)) != sizeof(msg))
> +		err("read() failed");

[Severity: Medium]
In uffd_wp_copy_reregister_test(), src_uffd is opened with uffd_open(O_CLOEXEC)
without O_NONBLOCK (unlike UFFD_FLAGS, which includes O_CLOEXEC | O_NONBLOCK).

In the kernel, mm/userfaultfd.c:userfaultfd_poll() requires O_NONBLOCK to be
set on the file descriptor:

	if (unlikely(!(file->f_flags & O_NONBLOCK)))
		return EPOLLERR;

Because POLLERR is returned in pfd.revents regardless of pfd.events,
poll(&pfd, 1, 10000) immediately returns 1 with pfd.revents == POLLERR without
waiting for a page fault. The 10-second timeout is bypassed, and if no fault
occurs on the source page, the subsequent read() will block indefinitely.

Should src_uffd be opened with O_CLOEXEC | O_NONBLOCK (or UFFD_FLAGS &
~UFFD_USER_MODE_ONLY), and should pfd.revents also be checked for POLLIN?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102081140.201987-1-azpijr@gmail.com?part=2