Re: [PATCH v3 2/2] selftests/mm: verify droppable mappings cannot be locked

["David Hildenbrand (Arm)" <david@kernel.org>]

>>
>>> +    if (munlockall()) {
>>> +        ksft_test_result_fail("munlockall() %s\n", strerror(errno));
>>> +        return;
>>> +    }
>>> +
>>> +    map = mmap(NULL, 2 * page_size, PROT_READ | PROT_WRITE,
>>> +           MAP_ANONYMOUS | MAP_DROPPABLE, -1, 0);
>>> +    if (map == MAP_FAILED) {
>>> +        if (errno == EOPNOTSUPP)
>>> +            ksft_test_result_skip("%s: MAP_DROPPABLE not
>>> supported\n", __func__);
>>> +        else
>>> +            ksft_test_result_fail("mmap error: %s\n", strerror(errno));
>>> +        return;
>>> +    }
>>> +
>>> +    if (mlock2_(map, 2 * page_size, 0))
>> Weird, is "mlock2_" actually correct? (not "mlock2") ?
> 
> It's correct though mlock2 would also work since it's been in glibc for
> several years now. I just matched the existing tests. mlock2_ is a
> simple wrapper around syscall in tools/testing/selftests/mm/mlock2.h,
> and it was introduced when the mlock2 syscall was introduced. A trailing
> rather than a preceding underscore is...unfortunate.

Interesting ... and confusing :)

> 
> 
>>
>>> +        ksft_test_result_fail("mlock2(0): %s\n", strerror(errno));
>>> +    else
>>> +        ksft_test_result(!unlock_lock_check(map, false),
>>> +                "%s: droppable memory not locked\n", __func__);
>>> +
>>> +    munmap(map, 2 * page_size);
>>> +}
>>> +
>>> +static void test_mlockall_future_droppable(void)
>>> +{
>>> +    char *map;
>>> +    unsigned long page_size = getpagesize();
>>> +
>>> +    if (mlockall(MCL_CURRENT | MCL_FUTURE)) {
>>> +        ksft_test_result_fail("mlockall(MCL_CURRENT | MCL_FUTURE):
>>> %s\n", strerror(errno));
>>> +        return;
>>> +    }
>>> +
>>> +    map = mmap(NULL, 2 * page_size, PROT_READ | PROT_WRITE,
>>> +           MAP_ANONYMOUS | MAP_DROPPABLE, -1, 0);
>>> +
>>> +    if (map == MAP_FAILED) {
>>> +        if (errno == EOPNOTSUPP)
>>> +            ksft_test_result_skip("%s: MAP_DROPPABLE not
>>> supported\n", __func__);
>>> +        else
>>> +            ksft_test_result_fail("mmap error: %s\n", strerror(errno));
>>> +        munlockall();
>>> +        return;
>>> +    }
>>> +
>>> +    ksft_test_result(!unlock_lock_check(map, false), "%s: droppable
>>> memory not locked\n",
>>> +            __func__);
>>> +
>>> +    munlockall();
>>>       munmap(map, 2 * page_size);
>>>   }
>>> +#else
>>> +static void test_mlock_droppable(void)
>>> +{
>>> +    ksft_test_result_skip("%s: MAP_DROPPABLE not supported\n",
>>> __func__);
>>> +}
>>> +
>>> +static void test_mlockall_future_droppable(void)
>>> +{
>>> +    ksft_test_result_skip("%s: MAP_DROPPABLE not supported\n",
>>> __func__);
>>> +}
>>> +#endif /* MAP_DROPPABLE */
>>>   
>> Why not a above a
>>
>> #ifndef MAP_DROPPABLE
>> #define MAP_DROPPABLE    0x08
>> #endif
>>
>> instead?
> 
> The intent was to skip the tests if compiled with headers where
> MAP_DROPPABLE isn't defined rather than force the value and get EINVAL
> because the kernel doesn't know about it. This way EINVAL can be flagged
> as a test failure and not skipped since it would likely indicate a test
> or kernel bug.

Note that the kernel headers you are compiling against don't imply
anything about the kernel that is actually running! So the argument
regarding EINVAL doesn't really hold.

But note that we, in general, try to compile against the in-tree headers.

See

commit 75d60eb30daafb966db0e45f38e4cdeb5e5ed79c
Author: Lorenzo Stoakes <ljs@kernel.org>
Date:   Mon Oct 28 14:13:30 2024 +0000

    tools: testing: update tools UAPI header for mman-common.h

    Import the new MADV_GUARD_INSTALL/REMOVE madvise flags.

And looking into it, I already see MAP_DROPPABLE there as well, so is
any special handling here even needed?

-- 
Cheers,

David