Message-ID: <8c94faf4-9af9-4d43-a597-6b06dd21be95@lucifer.local>
Date: Fri, 9 May 2025 10:49:47 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        linux-kselftest@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Shuah Khan <shuah@...nel.org>, Ingo Molnar <mingo@...hat.com>,
        Peter Xu <peterx@...hat.com>
Subject: Re: [PATCH v1] selftests/mm: add simple VM_PFNMAP tests based on
 mmap'ing /dev/mem

On Fri, May 09, 2025 at 12:20:41AM +0200, David Hildenbrand wrote:
> Let's test some basic functionality using /dev/mem. These tests will
> implicitly cover some PAT (Page Attribute Handling) handling on x86.

Ah this is really nice thanks for this!

/dev/mem is a good choice as a straightforward way to get VM_PFNMAP
mappings also.

>
> These tests will only run when /dev/mem access to the first two pages
> in physical address space is possible and allowed; otherwise, the tests
> are skipped.
>
> On current x86-64 with PAT inside a VM, all tests pass:
>
> 	TAP version 13
> 	1..19
> 	ok 1 madvise(MADV_DONTNEED) should be disallowed
> 	ok 2 madvise(MADV_DONTNEED_LOCKED) should be disallowed
> 	ok 3 madvise(MADV_FREE) should be disallowed
> 	ok 4 madvise(MADV_WIPEONFORK) should be disallowed
> 	ok 5 madvise(MADV_COLD) should be disallowed
> 	ok 6 madvise(MADV_PAGEOUT) should be disallowed
> 	ok 7 madvise(MADV_POPULATE_READ) should be disallowed
> 	ok 8 madvise(MADV_POPULATE_WRITE) should be disallowed
> 	ok 9 munmap() splitting
> 	ok 10 mmap() after splitting
> 	ok 11 mremap(MREMAP_FIXED)
> 	ok 12 mremap() shrinking
> 	ok 13 mremap() growing should be disallowed

One could argue these should be in mremap tests, but the mremap tests are a
bit of a mess and I think better here.

> 	ok 14 mprotect(PROT_NONE)
> 	ok 15 SIGSEGV expected
> 	ok 16 mprotect(PROT_READ)
> 	ok 17 SIGSEGV not expected
> 	ok 18 fork()
> 	ok 19 SIGSEGV in child not expected
> 	# Totals: pass:19 fail:0 xfail:0 xpass:0 skip:0 error:0

It'd be good to assert that merging doesn't work for VM_PFNMAP, though hm
one could argue that's not hugely useful as it's trivially implemented.

But I guess anything like that should live in merge.c.

>
> However, we are able to trigger:
>
> [   27.888251] x86/PAT: pfnmap:1790 freeing invalid memtype [mem 0x00000000-0x00000fff]
>
> There are probably more things worth testing in the future, such as
> MAP_PRIVATE handling. But this set of tests is sufficient to cover most of
> the things we will rework regarding PAT handling.
>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Shuah Khan <shuah@...nel.org>
> Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Peter Xu <peterx@...hat.com>
> Signed-off-by: David Hildenbrand <david@...hat.com>
> ---
>
> On current mm-unstable, the MADV_POPULATE_READ test fails because
> mm-unstable contains a patch [1] that must be dropped.
>
> [1] https://lore.kernel.org/all/20250507154105.763088-2-p.antoniou@partner.samsung.com/
>
> ---
>  tools/testing/selftests/mm/Makefile |   1 +
>  tools/testing/selftests/mm/pfnmap.c | 278 ++++++++++++++++++++++++++++

As Dev points out you should update .gitignore, but you should also update
run_vmtests.sh so this gets run with everything else!

>  2 files changed, 279 insertions(+)
>  create mode 100644 tools/testing/selftests/mm/pfnmap.c
>
> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
> index ad4d6043a60f0..ae6f994d3add7 100644
> --- a/tools/testing/selftests/mm/Makefile
> +++ b/tools/testing/selftests/mm/Makefile
> @@ -84,6 +84,7 @@ TEST_GEN_FILES += mremap_test
>  TEST_GEN_FILES += mseal_test
>  TEST_GEN_FILES += on-fault-limit
>  TEST_GEN_FILES += pagemap_ioctl
> +TEST_GEN_FILES += pfnmap
>  TEST_GEN_FILES += thuge-gen
>  TEST_GEN_FILES += transhuge-stress
>  TEST_GEN_FILES += uffd-stress
> diff --git a/tools/testing/selftests/mm/pfnmap.c b/tools/testing/selftests/mm/pfnmap.c
> new file mode 100644
> index 0000000000000..59be2f3221124
> --- /dev/null
> +++ b/tools/testing/selftests/mm/pfnmap.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Basic VM_PFNMAP tests relying on mmap() of '/dev/mem'
> + *
> + * Copyright 2025, Red Hat, Inc.
> + *
> + * Author(s): David Hildenbrand <david@...hat.com>
> + */
> +#define _GNU_SOURCE
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdint.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <signal.h>
> +#include <setjmp.h>
> +#include <linux/mman.h>
> +#include <sys/mman.h>
> +#include <sys/wait.h>
> +
> +#include "../kselftest.h"
> +#include "vm_util.h"
> +
> +static size_t pagesize;
> +static int pagemap_fd;
> +static int dev_mem_fd;
> +static sigjmp_buf env;
> +
> +static void signal_handler(int sig)
> +{
> +	if (sig == SIGSEGV)
> +		siglongjmp(env, 1);
> +	siglongjmp(env, 2);
> +}

Hm, wouldn't it be better to only catch these only if you specifically
meant to catch a signal?

You can see what I did in guard-regions.c for an example (sorry, I'm sure
you know exactly how the thing works, just I mean for an easy reminder :P)

> +
> +static void sense_support(void)
> +{

See below comment about the kselftest_harness, but with that you can
literally declare fixture setups/teardowns very nicely :) You can also
mmap() these 2 pages and munmap() them afterwards straightforwardly.

> +	char *addr, tmp;
> +	int ret;
> +
> +	dev_mem_fd = open("/dev/mem", O_RDONLY);
> +	if (dev_mem_fd < 0)
> +		ksft_exit_skip("Cannot open '/dev/mem': %s\n", strerror(errno));

Hm skip, or failure? Skip implies it's expected right? I suppose it's
possible a system might be setup without this...

> +
> +	/* We'll require the first two pages throughout our tests ... */
> +	addr = mmap(0, pagesize * 2, PROT_READ, MAP_SHARED, dev_mem_fd, 0);
> +	if (addr == MAP_FAILED)
> +		ksft_exit_skip("Cannot mmap '/dev/mem'");
> +
> +	/* ... and want to be able to read from them. */
> +	ret = sigsetjmp(env, 1);
> +	if (!ret) {
> +		tmp = *addr + *(addr + pagesize);
> +		asm volatile("" : "+r" (tmp));

Is this not pretty much equivalent to a volatile read where you're forcing
the compiler to not optimise this unused thing away? In guard-regions I set:

#define FORCE_READ(x) (*(volatile typeof(x) *)x)

For this purpose, which would make this:

FORCE_READ(addr);
FORCE_READ(&addr[pagesize]);

> +	}
> +	if (ret)
> +		ksft_exit_skip("Cannot read-access mmap'ed '/dev/mem'");

Why are we returning 1 or 2 if we don't differentiate it here?

> +
> +	munmap(addr, pagesize * 2);
> +}
> +
> +static void test_madvise(void)
> +{
> +#define INIT_ADVICE(nr) { nr, #nr}
> +	const struct {
> +		int nr;
> +		const char *name;
> +	} advices[] = {
> +		INIT_ADVICE(MADV_DONTNEED),
> +		INIT_ADVICE(MADV_DONTNEED_LOCKED),
> +		INIT_ADVICE(MADV_FREE),
> +		INIT_ADVICE(MADV_WIPEONFORK),
> +		INIT_ADVICE(MADV_COLD),
> +		INIT_ADVICE(MADV_PAGEOUT),
> +		INIT_ADVICE(MADV_POPULATE_READ),
> +		INIT_ADVICE(MADV_POPULATE_WRITE),
> +	};
> +	char *addr;
> +	int ret, i;
> +
> +	addr = mmap(0, pagesize, PROT_READ, MAP_SHARED, dev_mem_fd, 0);

Nit (same for all mmap() calls) shouldn't this first parameter be NULL, by
convention? I mean not a big deal obviously :)

> +	if (addr == MAP_FAILED)
> +		ksft_exit_fail_msg("mmap() failed: %s\n", strerror(errno));
> +
> +	/* All these advices must be rejected. */
> +	for (i = 0; i < ARRAY_SIZE(advices); i++) {
> +		ret = madvise(addr, pagesize, advices[i].nr);
> +		ksft_test_result(ret && errno == EINVAL,
> +				 "madvise(%s) should be disallowed\n",
> +				 advices[i].name);
> +	}
> +
> +	munmap(addr, pagesize);
> +}
> +
> +static void test_munmap_splitting(void)
> +{
> +	char *addr1, *addr2;
> +	int ret;
> +
> +	addr1 = mmap(0, pagesize * 2, PROT_READ, MAP_SHARED, dev_mem_fd, 0);
> +	if (addr1 == MAP_FAILED)
> +		ksft_exit_fail_msg("mmap() failed: %s\n", strerror(errno));
> +
> +	/* Unmap the first pages. */

NIT: pages -> page.

> +	ret = munmap(addr1, pagesize);
> +	ksft_test_result(!ret, "munmap() splitting\n");
> +
> +	/* Remap the first page while the second page is still mapped. */
> +	addr2 = mmap(0, pagesize, PROT_READ, MAP_SHARED, dev_mem_fd, 0);
> +	ksft_test_result(addr2 != MAP_FAILED, "mmap() after splitting\n");

Hm not sure what the assertion is here per se, that we can munmap() partial
bits of the VMA? It'd be pretty weird if we couldn't though?

If it's that we don't get a merge when we remap, we're not really checking
that, but you actually can, as I added an API to vm_util for this using
PROCMAP_QUERY (very handy tool actually - binary version of /proc/smaps).

You can then use that to determine if the VMAs are separate, see merge.c
for examples, it's actually really quite easy to use, e.g.:

	ASSERT_TRUE(find_vma_procmap(procmap, ptr));
	ASSERT_EQ(procmap->query.vma_start, (unsigned long)ptr);
	ASSERT_EQ(procmap->query.vma_end, (unsigned long)ptr + 10 * page_size);

> +
> +	if (addr2 != MAP_FAILED)
> +		munmap(addr2, pagesize);
> +	if (!ret)
> +		munmap(addr1 + pagesize, pagesize);
> +	else
> +		munmap(addr1, pagesize * 2);

There's no need for this dance, you can just munmap() away, it tolerates
gaps and multiple VMAs.

> +}
> +
> +static void test_mremap_fixed(void)
> +{
> +	char *addr, *new_addr, *ret;
> +
> +	addr = mmap(0, pagesize * 2, PROT_READ, MAP_SHARED, dev_mem_fd, 0);
> +	if (addr == MAP_FAILED)
> +		ksft_exit_fail_msg("mmap() failed: %s\n", strerror(errno));
> +
> +	/* Reserve a destination area. */
> +	new_addr = mmap(0, pagesize * 2, PROT_READ, MAP_ANON | MAP_PRIVATE, -1, 0);
> +	if (new_addr == MAP_FAILED)
> +		ksft_exit_fail_msg("mmap() failed: %s\n", strerror(errno));
> +
> +	/* mremap() over our destination. */
> +	ret = mremap(addr, pagesize * 2, pagesize * 2,
> +		     MREMAP_FIXED | MREMAP_MAYMOVE, new_addr);
> +	ksft_test_result(ret == new_addr, "mremap(MREMAP_FIXED)\n");
> +	if (ret != new_addr)
> +		munmap(new_addr, pagesize * 2);

This could only be an error code, and this will fail right?

MREMAP_FIXED is 'do or die' at the new address, not hinting. If there's
anything already mapped there it goes a bye bye.

So again, we could just have a standard munmap(), and this lends itself
well to a FIXTURE_SETUP()/FIXTURE_TEARDOWN() :P

> +	munmap(addr, pagesize * 2);
> +}
> +
> +static void test_mremap_shrinking(void)
> +{
> +	char *addr, *ret;
> +
> +	addr = mmap(0, pagesize * 2, PROT_READ, MAP_SHARED, dev_mem_fd, 0);
> +	if (addr == MAP_FAILED)
> +		ksft_exit_fail_msg("mmap() failed: %s\n", strerror(errno));
> +
> +	/* Shrinking is expected to work. */
> +	ret = mremap(addr, pagesize * 2, pagesize, 0);
> +	ksft_test_result(ret == addr, "mremap() shrinking\n");
> +	if (ret != addr)
> +		munmap(addr, pagesize * 2);
> +	else
> +		munmap(addr, pagesize);

I think we're safe to just munmap() as usual here :) (it's nitty but I'm
trying to make the case for teardown again of course :P)

> +}
> +
> +static void test_mremap_growing(void)
> +{
> +	char *addr, *ret;
> +
> +	addr = mmap(0, pagesize, PROT_READ, MAP_SHARED, dev_mem_fd, 0);
> +	if (addr == MAP_FAILED)
> +		ksft_exit_fail_msg("mmap() failed: %s\n", strerror(errno));
> +
> +	/* Growing is not expected to work. */

God imagine if we did allow it... what hell would it be to figure out how
to do this correctly in all cases :P

> +	ret = mremap(addr, pagesize, pagesize * 2, MREMAP_MAYMOVE);
> +	ksft_test_result(ret == MAP_FAILED,
> +			 "mremap() growing should be disallowed\n");
> +	if (ret == MAP_FAILED)
> +		munmap(addr, pagesize);
> +	else
> +		munmap(ret, pagesize * 2);

This is a bit cautious, for a world where we do lose our minds and allow
this? :)

> +}
> +
> +static void test_mprotect(void)
> +{
> +	char *addr, tmp;
> +	int ret;
> +
> +	addr = mmap(0, pagesize, PROT_READ, MAP_SHARED, dev_mem_fd, 0);
> +	if (addr == MAP_FAILED)
> +		ksft_exit_fail_msg("mmap() failed: %s\n", strerror(errno));
> +
> +	/* With PROT_NONE, read access must result in SIGSEGV. */
> +	ret = mprotect(addr, pagesize, PROT_NONE);
> +	ksft_test_result(!ret, "mprotect(PROT_NONE)\n");
> +
> +	ret = sigsetjmp(env, 1);
> +	if (!ret) {
> +		tmp = *addr;
> +		asm volatile("" : "+r" (tmp));
> +	}

This code is duplicated, we definitely want to abstract it.

> +	ksft_test_result(ret == 1, "SIGSEGV expected\n");

Hmm, what exactly are we testing here though? I mean PROT_NONE will be a
failed access for _any_ kind of memory? Is this really worthwhile? Maybe
better to mprotect() as PROT_NONE to start then mprotect() to PROT_READ.

But I'm not sure what that really tests? Is it a PAT-specific thing? It
seems if this is broken then the mapping code is more generally broken
beyond just VM_PFNMAP mappings right?

> +
> +	/* With PROT_READ, read access must again succeed. */
> +	ret = mprotect(addr, pagesize, PROT_READ);
> +	ksft_test_result(!ret, "mprotect(PROT_READ)\n");
> +
> +	ret = sigsetjmp(env, 1);
> +	if (!ret) {
> +		tmp = *addr;
> +		asm volatile("" : "+r" (tmp));
> +	}

And also duplicated here.

> +	ksft_test_result(!ret, "SIGSEGV not expected\n");
> +
> +	munmap(addr, pagesize);
> +}
> +
> +static void test_fork(void)
> +{
> +	char *addr, tmp;
> +	int ret;
> +
> +	addr = mmap(0, pagesize, PROT_READ, MAP_SHARED, dev_mem_fd, 0);
> +	if (addr == MAP_FAILED)
> +		ksft_exit_fail_msg("mmap() failed: %s\n", strerror(errno));
> +
> +	/* fork() a child and test if the child can access the page. */
> +	ret = fork();
> +	if (ret < 0) {
> +		ksft_test_result_fail("fork()\n");
> +		goto out;
> +	} else if (!ret) {
> +		ret = sigsetjmp(env, 1);
> +		if (!ret) {
> +			tmp = *addr;
> +			asm volatile("" : "+r" (tmp));
> +		}

Same comment as above re: duplication.

> +		/* Return the result to the parent. */
> +		exit(ret);
> +	}
> +	ksft_test_result_pass("fork()\n");
> +
> +	/* Wait for our child and obtain the result. */
> +	wait(&ret);
> +	if (WIFEXITED(ret))
> +		ret = WEXITSTATUS(ret);
> +	else
> +		ret = -EINVAL;
> +
> +	ksft_test_result(!ret, "SIGSEGV in child not expected\n");
> +out:
> +	munmap(addr, pagesize);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	int err;
> +
> +	ksft_print_header();
> +	ksft_set_plan(19);

I know it's kind of nitpicky, but I really hate this sort of magic number
and so on. You don't actually need any of this, the kselftest_harness.h is
_really_ powerful, and makes for much much more readable and standardised
test code.

You can look at guard-regions.c in the test code (though there's some
complexity there because I use 'variants') or the merge.c test code
(simpler) for straight-forward examples.

I won't block this change on this however, I don't want to be a pain and
you're adding very important tests here, but it'd be really nice if you did
use that :>)

> +
> +	pagesize = getpagesize();
> +	pagemap_fd = open("/proc/self/pagemap", O_RDONLY);
> +	if (pagemap_fd < 0)
> +		ksft_exit_fail_msg("opening pagemap failed\n");
> +	if (signal(SIGSEGV, signal_handler) == SIG_ERR)
> +		ksft_exit_fail_msg("signal() failed: %s\n", strerror(errno));
> +
> +	sense_support();
> +	test_madvise();
> +	test_munmap_splitting();
> +	test_mremap_fixed();
> +	test_mremap_shrinking();
> +	test_mremap_growing();
> +	test_mprotect();
> +	test_fork();
> +
> +	err = ksft_get_fail_cnt();
> +	if (err)
> +		ksft_exit_fail_msg("%d out of %d tests failed\n",
> +				   err, ksft_test_num());
> +	ksft_exit_pass();
> +}
> --
> 2.49.0
>

Powered by blists - more mailing lists