Re: [PATCH] selftests/harness: Drain the test process group after a timeout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Pre-existing process group initialization race leaves child process running concurrently on timeout.
- [High] Test child process and its process group are leaked if pidfd_open() or poll() fail.
--

commit 2f6bf81e41b5489d0a1cc6b990abec031b22d521
Author: Alex Williamson <alex.williamson@nvidia.com>

selftests/harness: Drain the test process group after a timeout

This commit updates the kselftest harness to actively wait for the entire
process group to exit when a test times out, bounded by a 5 second limit.
This addresses cascading failures caused by grandchild processes outliving
the timeout and holding onto exclusive resources.

> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 29a19bc870846..4e479c8cb49e8 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -981,8 +982,7 @@ static void __wait_for_test(struct __test_metadata *t)
>  	 */
>  	int status = KSFT_FAIL << 8;
>  	struct pollfd poll_child;
> -	int ret, child, childfd;
> -	bool timed_out = false;
> +	int ret, child = 0, childfd;
>  
>  	childfd = syscall(__NR_pidfd_open, t->pid, 0);
>  	if (childfd == -1) {

[Severity: High]
This is a pre-existing issue, but if syscall(__NR_pidfd_open) fails, the
function returns immediately. Can this leak the running child process and
its process group? If the leaked child holds exclusive resources like vfio
devices, could it cause subsequent tests to fail?

[ ... ]

> @@ -1004,9 +1004,46 @@ static void __wait_for_test(struct __test_metadata *t)
>  			t->name);
>  		return;

[Severity: High]
This is a pre-existing issue, but similarly, if poll() fails (e.g. returning
-1 with errno == EINTR), the function logs an error and returns. Doesn't
this also leak the child process and its process group by completely bypassing
the SIGKILL and waitpid() cleanup logic?

>  	} else if (ret == 0) {
> -		timed_out = true;
> +		int elapsed_ms = 0;
> +
>  		/* signal process group */
>  		kill(-(t->pid), SIGKILL);

[Severity: High]
This is a pre-existing issue, but is there a race condition here between the
parent calling kill() and the child calling setpgrp()?

If the child process created in __run_test() is delayed and hasn't executed
setpgrp() yet when this timeout occurs:

__run_test()
    child = fork();
    ...
    } else if (child == 0) {
        setpgrp();

Would this kill(-(t->pid), SIGKILL) fail with ESRCH because the process
group doesn't exist yet? This could result in the signal being lost, leaving
the child alive to run concurrently with subsequent tests once it is
scheduled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910230254.1198094-1-alex.williamson@nvidia.com?part=1