| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20250701030614.68902-1-sj@kernel.org>
Date: Mon, 30 Jun 2025 20:06:14 -0700
From: SeongJae Park <sj@...nel.org>
To: wang lian <lianux.mm@...il.com>
Cc: SeongJae Park <sj@...nel.org>,
david@...hat.com,
linux-mm@...ck.org,
akpm@...ux-foundation.org,
lorenzo.stoakes@...cle.com,
Liam.Howlett@...cle.com,
brauner@...nel.org,
gkwang@...x-info.com,
jannh@...gle.com,
linux-kernel@...r.kernel.org,
linux-kselftest@...r.kernel.org,
p1ucky0923@...il.com,
ryncsn@...il.com,
shuah@...nel.org,
vbabka@...e.cz,
zijing.zhang@...ton.me
Subject: Re: [PATCH v2] selftests/mm: Add process_madvise() tests
Hi Wang,
On Mon, 30 Jun 2025 22:09:57 +0800 wang lian <lianux.mm@...il.com> wrote:
> This patch adds tests for the process_madvise(), focusing on
> verifying behavior under various conditions including valid
> usage and error cases.
>
> Signed-off-by: wang lian<lianux.mm@...il.com>
> Suggested-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> Suggested-by: David Hildenbrand <david@...hat.com>
I left a few trivial comments below, but overall this looks useful to me.
Thank you :)
Acked-by: SeongJae Park <sj@...nel.org>
FYI, 'git am' of this patch on latest mm-new fails. Maybe this is not based on
the latest mm-new? But, I also found Andrew added this to mm tree:
https://lore.kernel.org/20250630230052.AB95CC4CEE3@smtp.kernel.org . Maybe
Andrew did rebase on his own?
[...]
> --- /dev/null
> +++ b/tools/testing/selftests/mm/process_madv.c
[...]
> +/* Try and read from a buffer, return true if no fatal signal. */
> +static bool try_read_buf(char *ptr)
> +{
> + return try_access_buf(ptr, false);
Seems this is the only caller of try_access_buf(). How about removing 'write'
argument of try_access_buf() and its handling?
> +}
> +
> +TEST_F(process_madvise, basic)
> +{
> + const unsigned long pagesize = (unsigned long)sysconf(_SC_PAGESIZE);
> + const int madvise_pages = 4;
> + char *map;
> + ssize_t ret;
> + struct iovec vec[madvise_pages];
> +
> + /*
> + * Create a single large mapping. We will pick pages from this
> + * mapping to advise on. This ensures we test non-contiguous iovecs.
> + */
> + map = mmap(NULL, pagesize * 10, PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> + ASSERT_NE(map, MAP_FAILED);
So this will make this test marked as failed, for mmap() failure, which doesn't
really mean something in process_madvise() is wrong. What about using
ksft_exit_skip() with failure check, instead?
> +
> + /* Fill the entire region with a known pattern. */
> + memset(map, 'A', pagesize * 10);
> +
> + /*
> + * Setup the iovec to point to 4 non-contiguous pages
> + * within the mapping.
> + */
> + vec[0].iov_base = &map[0 * pagesize];
> + vec[0].iov_len = pagesize;
> + vec[1].iov_base = &map[3 * pagesize];
> + vec[1].iov_len = pagesize;
> + vec[2].iov_base = &map[5 * pagesize];
> + vec[2].iov_len = pagesize;
> + vec[3].iov_base = &map[8 * pagesize];
> + vec[3].iov_len = pagesize;
> +
> + ret = sys_process_madvise(PIDFD_SELF, vec, madvise_pages, MADV_DONTNEED,
> + 0);
> + if (ret == -1 && errno == EPERM)
> + ksft_exit_skip(
> + "process_madvise() unsupported or permission denied, try running as root.\n");
> + else if (errno == EINVAL)
> + ksft_exit_skip(
> + "process_madvise() unsupported or parameter invalid, please check arguments.\n");
> +
> + /* The call should succeed and report the total bytes processed. */
> + ASSERT_EQ(ret, madvise_pages * pagesize);
> +
> + /* Check that advised pages are now zero. */
> + for (int i = 0; i < madvise_pages; i++) {
> + char *advised_page = (char *)vec[i].iov_base;
> +
> + /* Access should be successful (kernel provides a new page). */
> + ASSERT_TRUE(try_read_buf(advised_page));
> + /* Content must be 0, not 'A'. */
> + ASSERT_EQ(*advised_page, 0);
> + }
> +
> + /* Check that an un-advised page in between is still 'A'. */
> + char *unadvised_page = &map[1 * pagesize];
> +
> + ASSERT_TRUE(try_read_buf(unadvised_page));
> + ASSERT_EQ(*unadvised_page, 'A');
What about using memcpy() and memcmp() for testing full bytes? That could do
more complete test, and reduce unnecessary volatile and helper functions?
> +
> + /* Cleanup. */
> + ASSERT_EQ(munmap(map, pagesize * 10), 0);
Again, I think ksft_exit_skip() might be a better approach.
> +}
> +
> +static long get_smaps_anon_huge_pages(pid_t pid, void *addr)
> +{
> + char smaps_path[64];
> + char *line = NULL;
> + unsigned long start, end;
> + long anon_huge_kb;
> + size_t len;
> + FILE *f;
> + bool in_vma;
> +
> + in_vma = false;
> + sprintf(smaps_path, "/proc/%d/smaps", pid);
I understand this is just a test code, but... What about using the safer one,
snprintf()?
> + f = fopen(smaps_path, "r");
> + if (!f)
> + return -1;
> +
> + while (getline(&line, &len, f) != -1) {
> + /* Check if the line describes a VMA range */
> + if (sscanf(line, "%lx-%lx", &start, &end) == 2) {
> + if ((unsigned long)addr >= start &&
> + (unsigned long)addr < end)
> + in_vma = true;
> + else
> + in_vma = false;
> + continue;
> + }
> +
> + /* If we are in the correct VMA, look for the AnonHugePages field */
> + if (in_vma &&
> + sscanf(line, "AnonHugePages: %ld kB", &anon_huge_kb) == 1)
> + break;
> + }
> +
> + free(line);
> + fclose(f);
> +
> + return (anon_huge_kb > 0) ? (anon_huge_kb * 1024) : 0;
> +}
[...]
> +TEST_HARNESS_MAIN
> \ No newline at end of file
checkpatch.pl complaints having no newline at the end of the file.
> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> index f96d43153fc0..5c28ebcf1ea9 100755
> --- a/tools/testing/selftests/mm/run_vmtests.sh
> +++ b/tools/testing/selftests/mm/run_vmtests.sh
> @@ -61,6 +61,8 @@ separated by spaces:
> ksm tests that require >=2 NUMA nodes
> - pkey
> memory protection key tests
> +- process_madvise
Shouldn't this match with the real category name (process_madv) ?
> + test process_madvise
> - soft_dirty
> test soft dirty page bit semantics
> - pagemap
> @@ -424,6 +426,9 @@ CATEGORY="hmm" run_test bash ./test_hmm.sh smoke
> # MADV_GUARD_INSTALL and MADV_GUARD_REMOVE tests
> CATEGORY="madv_guard" run_test ./guard-regions
>
> +# PROCESS_MADVISE TEST
> +CATEGORY="process_madv" run_test ./process_madv
> +
> # MADV_DONTNEED and PROCESS_DONTNEED tests
> CATEGORY="madv_dontneed" run_test ./madv_dontneed
>
> --
> 2.43.0
Thanks,
SJ
Powered by blists - more mailing lists