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

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

[Muhammad Mahdi Amirpour]

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>
---
 tools/testing/selftests/mm/khugepaged.c | 57 +++++++++++++++++++++----
 1 file changed, 48 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/mm/khugepaged.c b/tools/testing/selftests/mm/khugepaged.c
index 1bc7acc66a46..2dd68dde1c14 100644
--- a/tools/testing/selftests/mm/khugepaged.c
+++ b/tools/testing/selftests/mm/khugepaged.c
@@ -1007,6 +1007,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);
@@ -1019,7 +1020,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");
@@ -1034,9 +1036,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;
 
@@ -1054,11 +1067,13 @@ static void collapse_fork(struct collapse_context *c, struct mem_ops *ops)
 static void collapse_fork_compound(struct collapse_context *c, struct mem_ops *ops)
 {
 	int wstatus;
+	pid_t pid;
 	void *p;
 
 	p = alloc_hpage(ops);
 	ksft_print_msg("Share huge 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, 1, hpage_pmd_size))
 			success("OK");
@@ -1084,9 +1099,20 @@ static void collapse_fork_compound(struct collapse_context *c, struct mem_ops *o
 		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;
+
+		fail("waitpid() failed");
+		goto out;
+	}
 
-	wait(&wstatus);
-	exit_status = WEXITSTATUS(wstatus);
+	exit_status = WIFEXITED(wstatus) ? WEXITSTATUS(wstatus) : KSFT_FAIL;
 	if (exit_status == KSFT_FAIL)
 		goto out;
 
@@ -1105,11 +1131,13 @@ static void collapse_max_ptes_shared(struct collapse_context *c, struct mem_ops
 {
 	int max_ptes_shared = thp_read_num("khugepaged/max_ptes_shared");
 	int wstatus;
+	pid_t pid;
 	void *p;
 
 	p = alloc_hpage(ops);
 	ksft_print_msg("Share huge 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, 1, hpage_pmd_size))
 			success("OK");
@@ -1145,9 +1173,20 @@ static void collapse_max_ptes_shared(struct collapse_context *c, struct mem_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;
+
+		fail("waitpid() failed");
+		goto out;
+	}
 
-	wait(&wstatus);
-	exit_status = WEXITSTATUS(wstatus);
+	exit_status = WIFEXITED(wstatus) ? WEXITSTATUS(wstatus) : KSFT_FAIL;
 	if (exit_status == KSFT_FAIL)
 		goto out;
 
-- 
2.55.0

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

[sashiko-bot]

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