| 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
| ||
|
Date: Mon, 2 Jan 2023 14:32:55 +0100
From: David Hildenbrand <david@...hat.com>
To: Lorenzo Stoakes <lstoakes@...il.com>, linux-mm@...ck.org,
Andrew Morton <akpm@...ux-foundation.org>,
Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org,
linux-kernel@...r.kernel.org
Cc: Vlastimil Babka <vbabka@...e.cz>,
Jakub Matena <matenajakub@...il.com>,
Matthew Wilcox <willy@...radead.org>,
Mel Gorman <mgorman@...hsingularity.net>,
Michal Hocko <mhocko@...nel.org>
Subject: Re: [PATCH] selftest/vm: add mremap expand merge offset test
On 16.12.22 22:44, Lorenzo Stoakes wrote:
> Add a test to assert that we can mremap() and expand a mapping starting
> from an offset within an existing mapping. We unmap the last page in a 3
> page mapping to ensure that the remap should always succeed, before
> remapping from the 2nd page.
>
> This is additionally a regression test for the issue solved in "mm, mremap:
> fix mremap() expanding vma with addr inside vma" and confirmed to fail
> prior to the change and pass after it.
>
> Finally, this patch updates the existing mremap expand merge test to check
> error conditions and reduce code duplication between the two tests.
>
> Signed-off-by: Lorenzo Stoakes <lstoakes@...il.com>
> ---
> tools/testing/selftests/vm/mremap_test.c | 111 +++++++++++++++++++----
> 1 file changed, 93 insertions(+), 18 deletions(-)
>
> diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
> index 9496346973d4..28a17d4e8afd 100644
> --- a/tools/testing/selftests/vm/mremap_test.c
> +++ b/tools/testing/selftests/vm/mremap_test.c
> @@ -119,30 +119,19 @@ static unsigned long long get_mmap_min_addr(void)
> }
>
> /*
> - * This test validates that merge is called when expanding a mapping.
> - * Mapping containing three pages is created, middle page is unmapped
> - * and then the mapping containing the first page is expanded so that
> - * it fills the created hole. The two parts should merge creating
> - * single mapping with three pages.
> + * Using /proc/self/maps, assert that the specified address range is contained
> + * within a single mapping.
> */
> -static void mremap_expand_merge(unsigned long page_size)
> +static bool is_range_mapped(void *start, void *end)
> {
> - char *test_name = "mremap expand merge";
> FILE *fp;
> char *line = NULL;
> size_t len = 0;
> bool success = false;
> - char *start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
> - MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> -
> - munmap(start + page_size, page_size);
> - mremap(start, page_size, 2 * page_size, 0);
>
> fp = fopen("/proc/self/maps", "r");
> - if (fp == NULL) {
> - ksft_test_result_fail("%s\n", test_name);
> - return;
> - }
> + if (fp == NULL)
> + return false;
This is unexpected. It would be valuable to ksft_print_msg("[INFO] .."
something, indicating that we don't know because we cannot access that info.
ksft_print_msg("[INFO] Opening /proc/self/maps failed"
But I'd even suggest opening "/proc/self/maps" once in main() and
failing directly there. Then we don't have to worry about it here.
>
> while (getline(&line, &len, fp) != -1) {
> char *first = strtok(line, "- ");
> @@ -150,16 +139,101 @@ static void mremap_expand_merge(unsigned long page_size)
> char *second = strtok(NULL, "- ");
> void *second_val = (void *) strtol(second, NULL, 16);
>
> - if (first_val == start && second_val == start + 3 * page_size) {
> + if (first_val <= start && second_val >= end) {
> success = true;
> break;
> }
> }
> +
> + fclose(fp);
> + return success;
> +}
> +
> +/*
> + * This test validates that merge is called when expanding a mapping.
> + * Mapping containing three pages is created, middle page is unmapped
> + * and then the mapping containing the first page is expanded so that
> + * it fills the created hole. The two parts should merge creating
> + * single mapping with three pages.
> + */
> +static void mremap_expand_merge(unsigned long page_size)
> +{
> + char *test_name = "mremap expand merge";
> + bool success = false;
> + int errsv = 0;
> + char *remap;
> + char *start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
I'd suggest
char *remap, *start;
start = mmap()
if (start == MAP_FAILED) { ...
to make this easier to read.
> +
> + if (start == MAP_FAILED) {
> + errsv = errno;
> + goto error;
> + }
> +
> + munmap(start + page_size, page_size);
> + remap = mremap(start, page_size, 2 * page_size, 0);
> + if (remap == MAP_FAILED) {
> + errsv = errno;
> + munmap(start, page_size);
> + munmap(start + 2 * page_size, page_size);
> + goto error;
> + }
> +
> + success = is_range_mapped(start, start + 3 * page_size);
> +
> + munmap(start, 3 * page_size);
> + goto out;
> +
> +error:
> + ksft_print_msg("Unexpected mapping/remapping error: %s\n",
> + strerror(errsv));
Please avoid the "error" label and just print proper errors directly at
the two callsites. Then, remove the "goto out".
> +out:
> + if (success)
> + ksft_test_result_pass("%s\n", test_name);
> + else
> + ksft_test_result_fail("%s\n", test_name);
> +}
> +
> +/*
> + * Similar to mremap_expand_merge() except instead of removing the middle page,
> + * we remove the last then attempt to remap offset from the second page. This
> + * should result in the mapping being restored to its former state.
> + */
> +static void mremap_expand_merge_offset(unsigned long page_size)
> +{
> +
> + char *test_name = "mremap expand merge offset";
> + bool success = false;
> + int errsv = 0;
> + char *remap;
> + char *start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
Dito.
> +
> + if (start == MAP_FAILED) {
> + errsv = errno;
> + goto error;
> + }
> +
> + /* Unmap final page to ensure we have space to expand. */
> + munmap(start + 2 * page_size, page_size);
> + remap = mremap(start + page_size, page_size, 2 * page_size, 0);
> + if (remap == MAP_FAILED) {
> + errsv = errno;
> + munmap(start, 2 * page_size);
> + goto error;
> + }
> +
> + success = is_range_mapped(start, start + 3 * page_size);
> + goto out;
> +
> +error:
> + ksft_print_msg("Unexpected mapping/remapping error: %s\n",
> + strerror(errsv));
Dito.
> +out:
--
Thanks,
David / dhildenb
Powered by blists - more mailing lists