On Sun, Apr 07, 2024 at 02:28:06AM +0500, Muhammad Usama Anjum wrote:
> On 4/5/24 1:44 PM, Dev Jain wrote:

> > + ksft_print_msg("%s :: %s\n", current->name, current->descr);
> > + if (test_setup(current) && test_init(current)) {
> > + test_run(current);
> > + test_cleanup(current);
> > + }
> > + test_result(current);
> > +
> > + return current->result;
> > +}

> This test isn't TAP compliant. Please make this and all tests TAP
> compilant. The 1/4 patch has example of TAP usage.

It's based on the 64 bit version of these tests which are also not TAP
compliant. TBH I'm not sure how worthwile it is to fix at all given
that they're all single test executables anyway, if it does get fixed
it'd be good to do the arm64 ones as well.

> > + } else {
> > + fprintf(stdout, "==>> completed. FAIL(0)\n");
> > + td->result = KSFT_FAIL;
> > + }
> > +
> > + if (force_exit)
> > + exit(td->result);
> > +}
> > +

Please delete unneeded context from mails when replying. Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

Attachments:

(No filename) (1.14 kB)
signature.asc (499.00 B)
Download all attachments

2024-04-08 12:24:25

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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>>> + *
>>> + * 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel