Re: [LTP] [RFC PATCH] syscalls/madvise: Check DONTNEED across an unmapped hole

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> Link: https://man7.org/linux/man-pages/man2/madvise.2.html
> Signed-off-by: Wenyuan Li <2063309626@qq.com>
> ---
>  runtest/syscalls                              |   1 +
>  testcases/kernel/syscalls/madvise/.gitignore  |   1 +
>  testcases/kernel/syscalls/madvise/madvise13.c | 145 ++++++++++++++++++
>  3 files changed, 147 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/madvise/madvise13.c
> 
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 4d3f53e..7e255c3 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1094,6 +1094,7 @@ madvise09 madvise09
>  madvise10 madvise10
>  madvise11 madvise11
>  madvise12 madvise12
> +madvise13 madvise13
>  
>  newuname01 newuname01
>  
> diff --git a/testcases/kernel/syscalls/madvise/.gitignore b/testcases/kernel/syscalls/madvise/.gitignore
> index 758e601..3e7dbe6 100644
> --- a/testcases/kernel/syscalls/madvise/.gitignore
> +++ b/testcases/kernel/syscalls/madvise/.gitignore
> @@ -9,3 +9,4 @@
>  /madvise10
>  /madvise11
>  /madvise12
> +/madvise13
> diff --git a/testcases/kernel/syscalls/madvise/madvise13.c b/testcases/kernel/syscalls/madvise/madvise13.c
> new file mode 100644
> index 0000000..eed99dc
> --- /dev/null
> +++ b/testcases/kernel/syscalls/madvise/madvise13.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2026 Wenyuan Li <2063309626@qq.com>
> + */
> +
> +/*\
> + * Verify MADV_DONTNEED on a private anonymous range containing an unmapped
> + * page. The call must return ENOMEM but still discard the mapped pages on
> + * both sides of the hole, without changing data outside the requested range.
> + * Also test the same request with the middle page mapped.
> + *
> + * madvise(2), VERSIONS/Linux, specifies that Linux applies the advice to
> + * the mapped parts of a range containing holes, even though it returns
> + * ENOMEM.
> + */
> +
> +#include <sys/mman.h>
> +#include "tst_test.h"
> +#include "lapi/mmap.h"
> +
> +#define PAGES 8
> +#define HOLE_PAGE 3
> +#define FIRST_PAGE 2
> +#define LAST_PAGE 4
> +
> +static unsigned char *addr;
> +static size_t page_size;
> +
> +static void cleanup(void)
> +{
> +	if (addr) {
> +		SAFE_MUNMAP(addr, PAGES * page_size);
> +		addr = NULL;
> +	}
> +}
> +
> +static void check_page(unsigned int page, unsigned char expected, int prepare)
> +{
> +	unsigned char vec;
> +	unsigned char *ptr = addr + page * page_size;
> +	size_t i;
> +
> +	TEST(mincore(ptr, page_size, &vec));
> +	if (TST_RET == -1) {
> +		if (prepare || TST_ERR != ENOMEM)
> +			tst_brk(TBROK | TTERRNO, "mincore() on page %u", page);
> +
> +		tst_res(TFAIL, "Page %u is no longer mapped", page);
> +		return;
> +	}
> +
> +	for (i = 0; i < page_size; i++) {
> +		if (ptr[i] == expected)
> +			continue;
> +
> +		if (prepare)
> +			tst_brk(TBROK, "Page %u was not initialized", page);
> +
> +		tst_res(TFAIL, "Page %u byte %zu: expected %#x, got %#x",
> +			page, i, expected, ptr[i]);
> +		return;
> +	}
> +
> +	if (!prepare)
> +		tst_res(TPASS, "Page %u contains only %#x", page, expected);
> +}
> +
> +static void check_hole(int prepare)
> +{
> +	unsigned char vec;
> +
> +	TEST(mincore(addr + HOLE_PAGE * page_size, page_size, &vec));
> +	if (TST_RET == -1 && TST_ERR == ENOMEM) {
> +		if (!prepare)
> +			tst_res(TPASS, "The hole remains unmapped");
> +		return;
> +	}
> +
> +	if (TST_RET == -1)
> +		tst_brk(TBROK | TTERRNO, "mincore() on the hole");
> +
> +	if (prepare)
> +		tst_brk(TBROK, "The hole was not created");
> +
> +	tst_res(TFAIL, "The hole is mapped after MADV_DONTNEED");
> +}

I find these functions with prepare parameter rather confusing to read.
I guess that it would make more sense to return if the page is
mapped/unmapped and produce the TPASS/TFAIL in the caller.

> +static void run(unsigned int hole)
> +{
> +	unsigned int page;
> +	unsigned char expected;
> +
> +	tst_res(TINFO, "MADV_DONTNEED %s a hole", hole ? "across" : "without");
> +	addr = SAFE_MMAP(NULL, PAGES * page_size, PROT_READ | PROT_WRITE,
> +			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +
> +	/* EINVAL is also returned when THP support is not configured. */
> +	if (madvise(addr, PAGES * page_size, MADV_NOHUGEPAGE) &&
> +	    errno != EINVAL)
> +		tst_brk(TBROK | TERRNO, "madvise(MADV_NOHUGEPAGE)");

Why do we need this? I suppose that even if kernel allocated a hugepage
it will be broken apart once we modify the middle. The THP stays for
transparent huge pages, hence it shouldn't be visible to the userspace
in any way.

> +	for (page = 0; page < PAGES; page++) {
> +		memset(addr + page * page_size, 0x40 + page, page_size);
> +		check_page(page, 0x40 + page, 1);
> +	}
> +
> +	if (hole) {
> +		SAFE_MUNMAP(addr + HOLE_PAGE * page_size, page_size);
> +		check_hole(1);
> +		TST_EXP_FAIL(madvise(addr + FIRST_PAGE * page_size,
> +			3 * page_size, MADV_DONTNEED), ENOMEM);
> +	} else {
> +		TST_EXP_PASS(madvise(addr + FIRST_PAGE * page_size,
> +			3 * page_size, MADV_DONTNEED));

Here we hardcode 3 as a number of pages to drop, but we define both
FIRST_PAGE and LAST_PAGE. It would make sense to define it as:

#define MADV_FIRST 2
#define MADV_LAST 4
#define MADV_LEN (MADV_LAST-MADV_FIRST+1)

> +	}
> +
> +	for (page = 0; page < PAGES; page++) {
> +		if (hole && page == HOLE_PAGE)
> +			continue;
> +
> +		expected = 0x40 + page;
> +		if (page >= FIRST_PAGE && page <= LAST_PAGE)
> +			expected = 0;
> +		check_page(page, expected, 0);
> +	}
> +
> +	if (hole)
> +		check_hole(0);
> +
> +	cleanup();
> +}
> +
> +static void setup(void)
> +{
> +	page_size = SAFE_SYSCONF(_SC_PAGESIZE);
> +	if (!page_size)
> +		tst_brk(TBROK, "Invalid page size");

This hardening should go directly into the safe_sysconf() in the
library. There are at least 30 tests that call SAFE_SYSCONF() with
_SC_PAGESIZE and it does not make sense to add these kind of checks into
each of them. And when we are at it we should probably check that the
page size is power of two as well.

Also safe_sysconf() should check for negative values other than -1.

> +}
> +
> +static struct tst_test test = {
> +	.test = run,
> +	.tcnt = 2,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +};
> -- 
> 2.43.0
> 
> base-commit: 46688815513714a28fd2ae300adae6b6e68b838b
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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