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

[David Hildenbrand (Arm)]

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@xxxxxxxxxx>
> ---


[...]

> +
> +#ifdef MAP_DROPPABLE
> +/*
> + * Droppable memory should not be lockable.
> + */

Single-line comment.

> +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") ?

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

>  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@xxxxxxxxxx>

-- 
Cheers,

David