Re: [PATCH] selftests/mm: pkeys: wait for the child in become_child()

[Hongfu Li <hongfu.li@linux.dev>]

在 11/7/26 00:46, Carlos López 写道:
> become_child() forks during test_pkey_alloc_exhaust() and has the parent
> exit, so that the remaining tests run in the child. However, the test
> runner only waits for the original process, so it sees the test finish
> early with a success status while the remaining tests keep running in
> the background. Failures after the first fork are thus not reported in
> the exit status, and the remaining tests overlap with the next selftest.
>
> Fix this by waiting for the child in the parent and exiting with its
> status. A previous fix avoided waiting, arguing that the parents would
> keep holding their resources [1]. Particularly, the parent keeps the
> hugetlb reservation of its test memory, so the child runs out of hugetlb
> pages after a few iterations. Avoid this by freeing the test memory
> before waiting.
>
> Link: https://lore.kernel.org/all/20240419115027.3848958-1-usama.anjum@collabora.com [1]
> Fixes: e1812933b17b ("x86/selftests/pkeys: Fork() to check for state being preserved")
> Signed-off-by: Carlos López <clopez@suse.de>

Hi Carlos,

This is a great catch, thank you for working on the fix.

I reproduced it locally: without the patch, a failure after become_child()
is not reflected in the result and the runner still prints
"ok 1 protection_keys_64"; with your patch the same failure is reported as
"not ok 1 protection_keys_64 # exit=1". So the fix works as described.

Tested-by: Hongfu Li <lihongfu@kylinos.cn>
Reviewed-by: Hongfu Li <lihongfu@kylinos.cn>

> ---
>   tools/testing/selftests/mm/protection_keys.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c
> index b7882ab97683..5dece144ab95 100644
> --- a/tools/testing/selftests/mm/protection_keys.c
> +++ b/tools/testing/selftests/mm/protection_keys.c
> @@ -1009,9 +1009,10 @@ static void test_pkey_syscalls_bad_args(int *ptr, u16 pkey)
>   	pkey_assert(err);
>   }
>   
> -static void become_child(void)
> +static void become_child(int *ptr)
>   {
>   	pid_t forkret;
> +	int status;
>   
>   	forkret = fork();
>   	pkey_assert(forkret >= 0);
> @@ -1021,7 +1022,16 @@ static void become_child(void)
>   		/* in the child */
>   		return;
>   	}
> -	_exit(0);
> +
> +	/*
> +	 * Wait for the child to finish the tests and pass on its result.
> +	 * Free the test memory first, as it may reserve pages from the
> +	 * small hugetlb pool that the child needs.
> +	 */
> +	free_pkey_malloc(ptr);
> +	if (waitpid(forkret, &status, 0) != forkret || !WIFEXITED(status))
> +		_exit(KSFT_FAIL);
> +	_exit(WEXITSTATUS(status));
>   }
>   
>   /* Assumes that all pkeys other than 'pkey' are unallocated */
> @@ -1060,7 +1070,7 @@ static void test_pkey_alloc_exhaust(int *ptr, u16 pkey)
>   		 * preserved across fork().
>   		 */
>   		if (i == NR_PKEYS*2)
> -			become_child();
> +			become_child(ptr);
>   	}
>   
>   	dprintf3("%s()::%d\n", __func__, __LINE__);
>
> base-commit: 843c58022865a7b0f9743416c2b48bc2632da23c

-- 
Best regards,
Hongfu