Re: [LTP] [PATCH] userfaultfd: Add test using UFFDIO_CONTINUE

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi Ricardo,

The commit message needs a body explaining why this test is being added
(e.g. UFFDIO_CONTINUE with minor shmem faults was previously untested)
and that the missing UFFD_PAGEFAULT_FLAG_MINOR fallback define is also
added.

> +#define _GNU_SOURCE
> +#include "config.h"

_GNU_SOURCE is not needed here. None of the other userfaultfd tests use
it and nothing in this test requires GNU extensions.

[...]

> +static int page_size;
> +static char *page;
> +static int uffd;
> +static int memfd = -1;

uffd must be initialized to -1. Zero is a valid fd (stdin).

[...]

> +static void reset_pages(void)
> +{
> +	if (page) {
> +		SAFE_MUNMAP(page, page_size);
> +		page = NULL;
> +	}
> +
> +	if (memfd >= 0) {

Use `if (memfd != -1)` -- that is the LTP fd-check convention.

Also, reset_pages() is the .cleanup callback but never closes uffd. If
run() aborts after SAFE_USERFAULTFD() but before spawning the thread
(e.g. TCONF at the feature check), uffd leaks. Add:

	if (uffd != -1)
		SAFE_CLOSE(uffd);

[...]

> +	SAFE_IOCTL(uffd, UFFDIO_CONTINUE, &uffdio_continue);
> +
> +	close(uffd);
> +	return NULL;

Use SAFE_CLOSE(uffd) instead of close(uffd).

[...]

> +	uffd = SAFE_USERFAULTFD(O_CLOEXEC | O_NONBLOCK, false);

Consider passing true for the retry parameter. The test does not need
privileged userfaultfd -- UFFD_FEATURE_MINOR_SHMEM works fine with
UFFD_USER_MODE_ONLY. With false, the test unnecessarily TCONFs on
systems where unprivileged_userfaultfd is disabled.

[...]

> +	uffdio_continue.range.start =
> +		msg.arg.pagefault.address & ~(page_size - 1);

page_size is int, so ~(page_size - 1) produces a signed int mask that
gets sign-extended when applied to the __u64 address. Cast to avoid
relying on implementation-defined behavior:

	msg.arg.pagefault.address & ~((unsigned long)page_size - 1);

[...]

> +	.needs_kconfigs = (const char *[]) {
> +		"CONFIG_HAVE_ARCH_USERFAULTFD_WP=y",
> +		NULL
> +	},

This test uses UFFD_FEATURE_MINOR_SHMEM, not write-protect.
CONFIG_HAVE_ARCH_USERFAULTFD_WP will wrongly skip the test on
architectures that support minor faults but not WP. Use
CONFIG_USERFAULTFD=y instead. The version gate is already handled by
.min_kver = "5.14".

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp