Re: [PATCH] selftests/mm: handle child status safely in khugepaged

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/mm: missing fail() on child signal death corrupts TAP output
--

commit bd942a41aed26d067e3b1b9aac04d7c243204d0b
Author: Muhammad Mahdi Amirpour <ampr.mhd@gmail.com>

selftests/mm: handle child status safely in khugepaged

This patch updates collapse_fork(), collapse_fork_compound(), and
collapse_max_ptes_shared() in the khugepaged selftest to check fork() and
waitpid() return values and verify WIFEXITED(wstatus) before reading
WEXITSTATUS(wstatus).

>     selftests/mm: handle child status safely in khugepaged
>     
>     The khugepaged selftest reports child exit status through a shared
>     variable without checking that the child actually ran to completion.
>     When fork() fails or the child is killed by a signal, the status is
>     undefined and the test can report a false pass.
>     
>     Capture the waitpid() result and distinguish normal exit, signal
>     death, and fork failure before interpreting the status. Report the
>     child's actual termination cause in the TAP output so failures are
>     visible rather than silently masked.
>     
>     Signed-off-by: Muhammad Mahdi Amirpour <ampr.mhd@gmail.com>

> diff --git a/tools/testing/selftests/mm/khugepaged.c b/tools/testing/selftests/mm/khugepaged.c
> index c81e0a6c6c54e..8de9b1746e632 100644
> --- a/tools/testing/selftests/mm/khugepaged.c
> +++ b/tools/testing/selftests/mm/khugepaged.c
> @@ -1009,6 +1009,7 @@ static void collapse_compound_extreme(struct collapse_context *c, struct mem_ops
>  static void collapse_fork(struct collapse_context *c, struct mem_ops *ops)
>  {
>  	int wstatus;
> +	pid_t pid;
>  	void *p;
>  
>  	p = ops->setup_area(1);
> @@ -1021,7 +1022,8 @@ static void collapse_fork(struct collapse_context *c, struct mem_ops *ops)
>  		fail("Fail");
>  
>  	ksft_print_msg("Share small page over fork()...");
> -	if (!fork()) {
> +	pid = fork();
> +	if (pid == 0) {
>  		/* Do not touch settings on child exit */
>  		if (ops->check_huge(p, hpage_pmd_size, 0, hpage_pmd_size))
>  			success("OK");
> @@ -1036,9 +1038,20 @@ static void collapse_fork(struct collapse_context *c, struct mem_ops *ops)
>  		ops->cleanup_area(p, hpage_pmd_size);
>  		_exit(exit_status);
>  	}
> +	if (pid < 0) {
> +		fail("fork() failed");
> +		goto out;
> +	}
> +
> +	while (waitpid(pid, &wstatus, 0) == -1) {
> +		if (errno == EINTR)
> +			continue;
>  
> -	wait(&wstatus);
> -	exit_status = WEXITSTATUS(wstatus);
> +		fail("waitpid() failed");
> +		goto out;
> +	}
> +
> +	exit_status = WIFEXITED(wstatus) ? WEXITSTATUS(wstatus) : KSFT_FAIL;
>  	if (exit_status == KSFT_FAIL)
>  		goto out;

[Severity: Medium]
In tools/testing/selftests/mm/khugepaged.c, stdout is unbuffered and
ksft_print_msg() prints a comment prefix and message without a trailing
newline, relying on a subsequent success(), fail(), or skip() call to
terminate the line.

In collapse_fork() (and identically in collapse_fork_compound() and
collapse_max_ptes_shared()), when fork() or waitpid() fails, the parent calls
fail() before jumping to out, which closes the open ksft_print_msg() line.

However, if the child is killed by a signal (!WIFEXITED(wstatus)) before
completing its open ksft_print_msg() step, setting exit_status = KSFT_FAIL and
jumping straight to out without calling fail() leaves that comment line
unterminated. When ksft_test_result_report() then runs at out:

    # Share small page over fork()...not ok 22 collapse_fork

Doesn't this append the test result line directly onto the unfinished comment
line in the output, causing parsers to treat the result as a comment?

Also, the commit message mentions that the patch distinguishes signal death
and reports the child's actual termination cause in the output (and states
that child exit status was previously reported through a shared variable
rather than wait(&wstatus)), but the code does not check WIFSIGNALED(wstatus)
or log the signal termination cause. Could a shared helper be used for the
fork/waitpid status check across collapse_fork(), collapse_fork_compound(),
and collapse_max_ptes_shared() that calls fail() when the child terminates
abnormally?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030215841.229969-1-ampr.mhd@gmail.com?part=1