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

[anthony.yznaga@oracle.com]

On 4/10/26 12:57 AM, David Hildenbrand (Arm) wrote:
> On 4/10/26 01:49, Anthony Yznaga wrote:
>> For configs that support MAP_DROPPABLE verify that a mapping created
>> with MAP_DROPPABLE cannot be locked via mlock(), and that it will not
>> be locked if it's created after mlockall(MCL_FUTURE).
>>
>> Signed-off-by: Anthony Yznaga <anthony.yznaga@oracle.com>
>> ---
>
> [...]
>
>> +
>> +#ifdef MAP_DROPPABLE
>> +/*
>> + * Droppable memory should not be lockable.
>> + */
> Single-line comment.

Okay.


>
>> +static void test_mlock_droppable(void)
>> +{
>> +	char *map;
>> +	unsigned long page_size = getpagesize();
> We could store that in a static global and query it once during main().
> Feel free to keep it as is.
>
>> +
>> +	/*
>> +	 * Ensure MCL_FUTURE is not set.
>> +	 */
> Dito.
>
>> +	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.


>
>> +		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.


>
>>   static void test_vma_management(bool call_mlock)
>>   {
>> @@ -442,7 +522,7 @@ int main(int argc, char **argv)
>>   
>>   	munmap(map, size);
>>   
>> -	ksft_set_plan(13);
>> +	ksft_set_plan(15);
>>   
>>   	test_mlock_lock();
>>   	test_mlock_onfault();
>> @@ -451,6 +531,8 @@ int main(int argc, char **argv)
>>   	test_lock_onfault_of_present();
>>   	test_vma_management(true);
>>   	test_mlockall();
>> +	test_mlock_droppable();
>> +	test_mlockall_future_droppable();
>>   
>>   	ksft_finished();
>>   }
> Feel free to add
>
> Acked-by: David Hildenbrand (Arm) <david@kernel.org>
>