Re: [PATCH 1/4] selftests/arm: Add mm test

[Dev Jain]

On 4/10/24 09:45, Dev Jain wrote:
> On 4/7/24 02:53, Muhammad Usama Anjum wrote:
> > On 4/5/24 1:44 PM, Dev Jain wrote:
> > > This patch tests the 4GB VA restriction for 32-bit processes; it is 
> > > required
> > > to test the compat layer, whether the kernel knows that it is 
> > > running a 32-bit
> > > process or not. Chunks are allocated until the VA gets exhausted; 
> > > mmap must
> > > fail beyond 4GB. This is asserted against the VA mappings found
> > > in /proc/self/maps.
> > >
> > > Signed-off-by: Dev Jain <dev.jain@xxxxxxx>
> > > ---
> > >   tools/testing/selftests/arm/mm/compat_va.c | 94 
> > > ++++++++++++++++++++++
> > >   1 file changed, 94 insertions(+)
> > >   create mode 100644 tools/testing/selftests/arm/mm/compat_va.c
> > >
> > > diff --git a/tools/testing/selftests/arm/mm/compat_va.c 
> > > b/tools/testing/selftests/arm/mm/compat_va.c
> > > new file mode 100644
> > > index 000000000000..3a78f240bc87
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/arm/mm/compat_va.c
> > > @@ -0,0 +1,94 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2024 ARM Limited
> > > + *
> > > + * Author : Dev Jain <dev.jain@xxxxxxx>
> > > + *
> > > + * Tests 4GB VA restriction for 32 bit process
> > > + */
> > > +
> > > +#define _GNU_SOURCE
> > > +#include <stdio.h>
> > > +#include <stdlib.h>
> > > +#include <unistd.h>
> > > +#include <sys/mman.h>
> > > +
> > > +#include <linux/sizes.h>
> > > +#include <kselftest.h>
> > > +
> > > +#define MAP_CHUNK_SIZE    SZ_1M
> > > +#define NR_CHUNKS_4G    (SZ_1G / MAP_CHUNK_SIZE) * 4    /* prevent 
> > > overflow */
> > > +
> > > +static int validate_address_hint(void)
> > > +{
> > > +    char *ptr;
> > > +
> > > +    ptr = mmap((void *) (1UL << 29), MAP_CHUNK_SIZE, PROT_READ |
> > > +           PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > > +
> > > +    if (ptr == MAP_FAILED)
> > > +        return 0;
> > > +
> > > +    return 1;
> > Usually we return negative value instead of positive one which indicates
> > error situation.
> >
> > > +}
> > > +
> > > +int main(int argc, char *argv[])
> > > +{
> > > +    char *ptr[NR_CHUNKS_4G + 3];
> > > +    char line[1000];
> > > +    const char *file_name;
> > > +    int chunks;
> > > +    FILE *file;
> > > +    int i;
> > > +
> > > +    ksft_print_header();
> > > +    ksft_set_plan(1);
> > There are multiple test cases. Instead of saying there is only 1 test.
> > There should be multiple ksft_test_result{_pass,_fail} statements for 
> > each
> > sub-tests.
>
>
> My initial idea was to treat this as a single logical test, as I
>
> am asserting the restriction on the number of chunks against
>
> the VMAs. I guess your approach is cleaner; thanks.


Thinking again, using a lot of return statements is in fact making the 
code easier

to follow. If I just set a variable ret = 0/1 and use it to pass or 
fail, I'll have to

unnecessarily use a lot of if/else statements. Take a look at the 
examples below.

> > > +
> > > +    /* try allocation beyond 4 GB */
> > > +    for (i = 0; i < NR_CHUNKS_4G + 3; ++i) {
> > > +        ptr[i] = mmap(NULL, MAP_CHUNK_SIZE, PROT_READ | PROT_WRITE,
> > > +                  MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > > +
> > > +        if (ptr[i] == MAP_FAILED) {
> > > +            if (validate_address_hint())
> > > +                ksft_exit_fail_msg("VA exhaustion failed\n");
> > > +            break;


I will have to set ret value here, forcing two statements inside the if 
block.

> > > +        }
> > > +    }
> > > +
> > > +    chunks = i;
> > > +    if (chunks >= NR_CHUNKS_4G) {
> > > +        ksft_test_result_fail("mmapped chunks beyond 4GB\n");
> > > +        ksft_finished();
> > > +    }
> > > +
> > > +    /* parse /proc/self/maps, confirm 32 bit VA mappings */
> > > +    file_name = "/proc/self/maps";
> > > +    file = fopen(file_name, "r");
> > > +    if (file == NULL)
> > > +        ksft_exit_fail_msg("/proc/self/maps cannot be opened\n");
> > > +


I will have to set ret here, and enclose the below while statement inside

an else block. In short, saving the value of ret will require if/else blocks

if I were to use it in the end in ksft_test_result(). When I use ksft_exit

statements, it is clear that a problem was spotted here, and there is

no need to study the remaining code.

> > > +    while (fgets(line, sizeof(line), file)) {
> > > +        const char *whitespace_loc, *hyphen_loc;
> > > +
> > > +        hyphen_loc = strchr(line, '-');
> > > +        whitespace_loc = strchr(line, ' ');
> > > +
> > > +        if (!(hyphen_loc && whitespace_loc)) {
> > > +            ksft_test_result_skip("Unexpected format");
> > > +            ksft_finished();
> > I'm unable to follow as there are too many return statements. If you 
> > divide
> > the test into multiple sub-tests, you can skip/pass/fail each 
> > sub-test easily.
> >
> > > +        }
> > > +
> > > +        if ((hyphen_loc - line > 8) ||
> > > +            (whitespace_loc - hyphen_loc) > 9) {
> > > +            ksft_test_result_fail("Memory map more than 32 bits\n");
> > > +            ksft_finished();
> > > +        }
> > > +    }
> > > +
> > > +    for (int i = 0; i < chunks; ++i)
> > > +        munmap(ptr[i], MAP_CHUNK_SIZE);
> > > +
> > > +    ksft_test_result_pass("Test\n");
> > > +    ksft_finished();
> > > +}
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel