On 09.05.25 11:49, Lorenzo Stoakes wrote:
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!
Thanks for your review!
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.
I assume we'd need is_range_mapped() from mremap_tests.c.
Something for another day :)
[...]
+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?
I had that, but got tired about the repeated register + unregister, after all I really don't want to spend a lot more time on this.
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)
Again, time is the limit. But let me see if I can get something done in a reasonable timeframe.
+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...
Try as non-root or on a lockdowned system :)
- /* 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]);
Hmmm, a compiler might be allowed to optimize out a volatile read.
- }
- 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?
Copy-and-paste. As we are not registering for SIGBUS, we can just return 1.
- 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 :)
Yes.
- 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.
Ack.
- 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).
I don't care about merging tests (I'll leave that to you :P ).
This is a PAT test for upcoming changes where partial unmap can leave the original region reserved. Making sure that re-mapping with the pending reservation still works.
- 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.
Yeah, I know. I was not sure if the ksft_test_result() in between might allocate memory and consume that area.
+}
+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
I'm afraid I cannot spend much more time on these tests :P But let me try for a couple of minutes.
- 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)
Same reasoning as above regarding ksft_test_result().
+}
+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? :)
Yeah, went back and forth with this error cleanup shit.
+}
+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.
Probably yes.
- 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?
Rationale was to test the !vm_normal_folio() code paths that are not covered by "ordinary" mprotect (except the shared zeropage). But there should indeed only be such a check on the prot_numa code path, so I can just drop this test.
[...]
+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 :>)
Yeah, let me explore that real quick, thanks!