Re: [LTP] [PATCH v3] mremap07.c: New case check mremap with MREMAP_DONTUNMAP

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

> This case test mremap() with MREMAP_DONTUNMAP and use userfaultfd
> verifies fault which triggered by accessing old memory region.

nit: Having a changelog would help reviewing.

...
> +++ b/configure.ac
> @@ -46,6 +46,7 @@ AC_CHECK_DECLS([MADV_MERGEABLE],,,[#include <sys/mman.h>])
>  AC_CHECK_DECLS([NFTA_CHAIN_ID, NFTA_VERDICT_CHAIN_ID],,,[#include <linux/netfilter/nf_tables.h>])
>  AC_CHECK_DECLS([PR_CAPBSET_DROP, PR_CAPBSET_READ],,,[#include <sys/prctl.h>])
>  AC_CHECK_DECLS([SEM_STAT_ANY],,,[#include <sys/sem.h>])
> +AC_CHECK_DECLS([MREMAP_DONTUNMAP],,,[#include <linux/mman.h>])
Obviously AC_CHECK_DECLS() does not require to define _GNU_SOURCE, interesting.

...
> +++ b/testcases/kernel/syscalls/mremap/mremap07.c
> @@ -0,0 +1,161 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2025 Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * LTP test case for mremap() with MREMAP_DONTUNMAP and userfaultfd.
> + *
> + * This test verifies a fault is triggered on the old memory region
> + * and is then correctly handled by a userfaultfd handler.
> + */

You should include config.h if you want to check for HAVE_DECL_MREMAP_DONTUNMAP:
#include "config.h"

This works just because some header already do it, but that can change in the
future.

> +#define _GNU_SOURCE
> +#include <poll.h>
> +#include <pthread.h>
> +
> +#include "tst_test.h"
> +#include "tst_safe_pthread.h"
> +#include "lapi/userfaultfd.h"
> +
> +#if HAVE_DECL_MREMAP_DONTUNMAP

Interesting, you don't include <linux/mman.h>, which should have
MREMAP_DONTUNMAP, but the check works as expected. But I would still prefer to
include <linux/mman.h>.

> +static int page_size;
> +static int uffd;
> +static char *fault_addr;
> +static char *new_remap_addr;
> +
> +static const char *test_string = "Hello, world! This is a test string that fills up a page.";
> +
> +static int sys_userfaultfd(int flags)
> +{
> +	return tst_syscall(__NR_userfaultfd, flags);
> +}

This is copy pasted from userfaultfd01.c. I factored it out to
include/lapi/userfaultfd.h, could you please use it in the next version?

https://lore.kernel.org/ltp/20251030192543.761804-1-pvorel@suse.cz/
https://patchwork.ozlabs.org/project/ltp/patch/20251030192543.761804-1-pvorel@suse.cz/

> +static void fault_handler_thread(void)
> +{
> +	struct uffd_msg msg;
> +	struct uffdio_copy uffdio_copy;
> +
> +	TST_CHECKPOINT_WAIT(0);
> +
> +	struct pollfd pollfd;
> +
> +	pollfd.fd = uffd;
> +	pollfd.events = POLLIN;
> +
> +	int nready = poll(&pollfd, 1, -1);
Interesting, we still don't have safe_poll().

> +	if (nready <= 0)
man poll(2) says:
return value of zero indicates that the system call timed out before any file descriptors became ready.
userfaultfd01.c checks only for nready == -1, I'm not sure maybe it should also check for 0.
But if you also check for 0, maybe printing nready would be useful (OTOH TERRNO
prints SUCCESS(0)).

> +		tst_brk(TBROK | TERRNO, "Poll on uffd failed");
Maybe just poll() failed?

> +
> +	SAFE_READ(1, uffd, &msg, sizeof(msg));
> +
> +	if (msg.event != UFFD_EVENT_PAGEFAULT)
> +		tst_brk(TBROK, "Received unexpected UFFD_EVENT: %d", msg.event);
> +
> +	if ((char *)msg.arg.pagefault.address != fault_addr)
> +		tst_brk(TBROK, "Page fault on unexpected address: %p", (void *)msg.arg.pagefault.address);
> +
> +	tst_res(TINFO, "Userfaultfd handler caught a page fault at %p", (void *)msg.arg.pagefault.address);
> +
> +	uffdio_copy.src = (unsigned long)new_remap_addr;
> +	uffdio_copy.dst = (unsigned long)fault_addr;
> +	uffdio_copy.len = page_size;
> +	uffdio_copy.mode = 0;
> +	uffdio_copy.copy = 0;

Most of this code is the same as in userfaultfd01.c, but I don't see a way to
factor it out more than what I sent in my patch.

> +
> +	SAFE_IOCTL(uffd, UFFDIO_COPY, &uffdio_copy);
> +	tst_res(TPASS, "Userfaultfd handler successfully handled the fault.");
very nit: please avoid '.' at the end.
> +}
> +
> +static void setup(void)
> +{
> +	page_size = getpagesize();
> +	struct uffdio_api uffdio_api;
> +	struct uffdio_register uffdio_register;
> +
> +	TEST(sys_userfaultfd(O_CLOEXEC | O_NONBLOCK));
> +	if (TST_RET == -1) {
> +		if (TST_ERR == EPERM) {
> +			tst_res(TCONF, "Hint: check /proc/sys/vm/unprivileged_userfaultfd");
> +			tst_brk(TCONF | TTERRNO, "userfaultfd() requires CAP_SYS_PTRACE on this system");
> +		} else {
> +			tst_brk(TBROK | TTERRNO, "Could not create userfault file descriptor");
> +		}
> +	}
This would be also replaced by my patch, only this would be used:
uffd = SAFE_USERFAULTFD(O_CLOEXEC | O_NONBLOCK, true);

> +
> +	uffd = TST_RET;
> +	uffdio_api.api = UFFD_API;
> +	uffdio_api.features = 0;
> +	SAFE_IOCTL(uffd, UFFDIO_API, &uffdio_api);
> +
> +	fault_addr = SAFE_MMAP(NULL, page_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
nit: maybe split long line?
> +
> +	tst_res(TINFO, "Original mapping created at %p", (void *)fault_addr);
> +
> +	strcpy(fault_addr, "ABCD");
> +
> +	uffdio_register.range.start = (unsigned long)fault_addr;
> +	uffdio_register.range.len = page_size;
> +	uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> +	SAFE_IOCTL(uffd, UFFDIO_REGISTER, &uffdio_register);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (new_remap_addr != NULL)
> +		SAFE_MUNMAP(new_remap_addr, page_size);
> +
> +	if (fault_addr != NULL)
> +		SAFE_MUNMAP(fault_addr, page_size);
> +
> +	if (uffd != -1)
> +		SAFE_CLOSE(uffd);
> +}
> +
> +static void run(void)
> +{
> +	pthread_t handler_thread;
> +
> +	SAFE_PTHREAD_CREATE(&handler_thread, NULL,
> +		(void * (*)(void *))fault_handler_thread, NULL);
> +
> +	new_remap_addr = mremap(fault_addr, page_size, page_size, MREMAP_DONTUNMAP | MREMAP_MAYMOVE);
nit: long line.

> +
> +	if (new_remap_addr == MAP_FAILED)
> +		tst_brk(TBROK | TTERRNO, "mremap failed");
> +
> +	tst_res(TINFO, "New mapping created at %p", (void *)new_remap_addr);
> +
> +	strcpy(new_remap_addr, test_string);
> +
> +	TST_CHECKPOINT_WAKE(0);
> +
> +	tst_res(TINFO, "Main thread accessing old address %p to trigger fault. Access Content is \"%s\"",
> +			(void *)fault_addr, fault_addr);
nit: could we remove "Access Content is \"%s\"" ? It's not important for
the result if it pass (you print it also on the failure) and line is too long.

> +
> +	SAFE_PTHREAD_JOIN(handler_thread, NULL);
> +
> +	if (strcmp(fault_addr, test_string) != 0)
> +		tst_res(TFAIL, "Verification failed: Content at old address is '%s', expected '%s'", fault_addr, test_string);
Maybe s/at/of the/ ?

> +	else
> +		tst_res(TPASS, "Verification passed: Content at old address is correct after fault handling.");
nit: '.' at the end.
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.needs_checkpoints = 1,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.needs_kconfigs = (const char *[]) {
> +		"CONFIG_USERFAULTFD=y",
> +		NULL,
> +	},
> +	.min_kver = "5.7",

I wonder if we check for MREMAP_DONTUNMAP whether we need to have also explicit
check for MREMAP_DONTUNMAP. But sure, it's safer to have runtime kernel check
and check that it was not compiled with an old headers.

Kind regards,
Petr

> +};
> +
> +#else
> +TST_TEST_TCONF("Missing MREMAP_DONTUNMAP in <linux/mman.h>");
> +#endif

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