Re: [PATCH v2] selftests/mm: add simple VM_PFNMAP tests based on mmap'ing /dev/mem

On 28.05.25 12:34, Ryan Roberts wrote:

Hi David,

On 09/05/2025 16:30, 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.

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.

We are seeing really horrible RAS errors with this test when run on arm64 tx2 machine. Based solely on reviewing the code, I think the problem is that tx2 doesn't have anything at phys address 0, so test_read_access() is trying to put trasactions out to a bad address on the bus.

tx2 /proc/iomem:

$ sudo cat /proc/iomem 30000000-37ffffff : PCI ECAM 38000000-3fffffff : PCI ECAM 40000000-5fffffff : PCI Bus 0000:00 ...

Whereas my x86 box has some reserved memory:

$ sudo cat /proc/iomem 00000000-00000fff : Reserved 00001000-0003dfff : System RAM ...

A quick fix would be to make this test specific to x86 (the only one I tested on). We should always have the lower two pages IIRC (BIOS stuff etc).

I think perhaps the only safe way to handle this is to parse /proc/iomem for a region of "System RAM" that is at least 2 pages then use that for your read tests. This would also solve the hypothetical issue of reading something that has read size effects.

That sounds also plausible yes. I somehow remembered that mmap() would fail if "there is nothing".

I also spotted a few nits while reading the code...

...

On current x86-64 with PAT inside a VM, all tests pass:

TAP version 13 1..6 # Starting 6 tests from 1 test cases. # RUN pfnmap.madvise_disallowed ... # OK pfnmap.madvise_disallowed ok 1 pfnmap.madvise_disallowed # RUN pfnmap.munmap_split ... # OK pfnmap.munmap_split ok 2 pfnmap.munmap_split # RUN pfnmap.mremap_fixed ... # OK pfnmap.mremap_fixed ok 3 pfnmap.mremap_fixed # RUN pfnmap.mremap_shrink ... # OK pfnmap.mremap_shrink ok 4 pfnmap.mremap_shrink # RUN pfnmap.mremap_expand ... # OK pfnmap.mremap_expand ok 5 pfnmap.mremap_expand # RUN pfnmap.fork ... # OK pfnmap.fork ok 6 pfnmap.fork # PASSED: 6 / 6 tests passed. # Totals: pass:6 fail:0 xfail:0 xpass:0 skip:0 error:0

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@linux-foundation.org Cc: Shuah Khan shuah@kernel.org Cc: Lorenzo Stoakes lorenzo.stoakes@oracle.com Cc: Ingo Molnar mingo@redhat.com Cc: Peter Xu peterx@redhat.com Cc: Dev Jain dev.jain@arm.com Signed-off-by: David Hildenbrand david@redhat.com

---

Hopefully I didn't miss any review feedback.

v1 -> v2:

• Rewrite using kselftest_harness, which simplifies a lot of things
• Add to .gitignore and run_vmtests.sh
• Register signal handler on demand
• Use volatile trick to force a read (not factoring out FORCE_READ just yet)
• Drop mprotect() test case
• Add some more comments why we test certain things
• Use NULL for mmap() first parameter instead of 0
• Smaller fixes

---

tools/testing/selftests/mm/.gitignore | 1 + tools/testing/selftests/mm/Makefile | 1 + tools/testing/selftests/mm/pfnmap.c | 196 ++++++++++++++++++++++ tools/testing/selftests/mm/run_vmtests.sh | 4 + 4 files changed, 202 insertions(+) create mode 100644 tools/testing/selftests/mm/pfnmap.c

diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore index 91db34941a143..824266982aa36 100644 --- a/tools/testing/selftests/mm/.gitignore +++ b/tools/testing/selftests/mm/.gitignore @@ -20,6 +20,7 @@ mremap_test on-fault-limit transhuge-stress pagemap_ioctl +pfnmap *.tmp* protection_keys protection_keys_32 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..8a9d19b6020c7 --- /dev/null +++ b/tools/testing/selftests/mm/pfnmap.c @@ -0,0 +1,196 @@ +// 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@redhat.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_harness.h" +#include "vm_util.h"

+static sigjmp_buf sigjmp_buf_env;

+static void signal_handler(int sig) +{

• siglongjmp(sigjmp_buf_env, -EFAULT);

+}

+static int test_read_access(char *addr, size_t size, size_t pagesize) +{

• size_t offs;
• int ret;
• if (signal(SIGSEGV, signal_handler) == SIG_ERR)

• return -EINVAL;
  

• ret = sigsetjmp(sigjmp_buf_env, 1);
• if (!ret) {

• for (offs = 0; offs < size; offs += pagesize)
  

• 	/* Force a read that the compiler cannot optimize out. */
  

• 	*((volatile char *)(addr + offs));
  

• }
• if (signal(SIGSEGV, signal_handler) == SIG_ERR)

I think you mean: if (signal(SIGSEGV, SIG_DFL) == SIG_ERR)

Otherwise your signal_handler will remain installed, right?

Yeah, copy and past error.

...

• return -EINVAL;
  

• return ret;

+}

+FIXTURE(pfnmap) +{

• size_t pagesize;
• int dev_mem_fd;
• char *addr1;
• size_t size1;
• char *addr2;
• size_t size2;

+};

+FIXTURE_SETUP(pfnmap) +{

• self->pagesize = getpagesize();
• self->dev_mem_fd = open("/dev/mem", O_RDONLY);
• if (self->dev_mem_fd < 0)

• SKIP(return, "Cannot open '/dev/mem'\n");
  

• /* We'll require the first two pages throughout our tests ... */
• self->size1 = self->pagesize * 2;
• self->addr1 = mmap(NULL, self->size1, PROT_READ, MAP_SHARED,

• 	   self->dev_mem_fd, 0);
  

• if (self->addr1 == MAP_FAILED)

• SKIP(return, "Cannot mmap '/dev/mem'\n");
  

• /* ... and want to be able to read from them. */
• if (test_read_access(self->addr1, self->size1, self->pagesize))

• SKIP(return, "Cannot read-access mmap'ed '/dev/mem'\n");
  

• self->size2 = 0;
• self->addr2 = MAP_FAILED;

Do you need to init all the params in FIXTURE(pfnmap) to their "safe" values before any possible early returns? We don't want FIXTURE_TEARDOWN(pfnmap) getting confused.

If FIXTURE_SETUP fails, we'll exit immediately. See __TEST_F_IMPL().

Note that all tests are executed in a fork'ed child process, so quitting early (or not even having FIXTURE_TEARDOWN in most cases ...) does not really matter.