On Mon, Jan 02, 2023 at 02:32:55PM +0100, David Hildenbrand wrote:
> 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@xxxxxxxxx>
> > ---
> > 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
>
Ack on all, will spin a v2. Thanks for review!
