Re: [PATCH sched_ext for-7.4] selftests/sched_ext: Fix rt_stall runner abort

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/sched_ext: file descriptor leaks in error paths
--

commit bd4cf2003fa0e397ca084d4257a65e3f8e741ed8
Author: Cheng-Yang Chou <yphbchou0911@gmail.com>

selftests/sched_ext: Fix rt_stall runner abort

This commit drops the ksft_exit_fail() usage in rt_stall in favor of returning
false, allowing the test runner to handle the failure. It also refactors the
cleanup paths to avoid leaving SCHED_FIFO tasks spinning on failure.

> diff --git a/tools/testing/selftests/sched_ext/rt_stall.c b/tools/testing/selftests/sched_ext/rt_stall.c
> index a5041fc2e44fd..a1552b38a6e8c 100644
> --- a/tools/testing/selftests/sched_ext/rt_stall.c
> +++ b/tools/testing/selftests/sched_ext/rt_stall.c
[ ... ]
> @@ -151,13 +152,11 @@ static bool sched_stress_test(bool is_ext)
>  	float ext_runtime, rt_runtime, actual_ratio;
>  	int ext_pid, rt_pid;
>  	int ext_ready[2], rt_ready[2];
> -
> -	ksft_print_header();
> -	ksft_set_plan(1);
> +	bool ret = false;
>  
>  	if (pipe(ext_ready) || pipe(rt_ready)) {
>  		perror("pipe");
> -		ksft_exit_fail();
> +		return false;
>  	}

[Severity: Medium]
If the first pipe creation succeeds but the second fails, does this early
return leak the ext_ready file descriptors?

[ ... ]
> @@ -172,7 +171,7 @@ static bool sched_stress_test(bool is_ext)
>  		exit(0);
>  	} else if (ext_pid < 0) {
>  		perror("fork task");
> -		ksft_exit_fail();
> +		return false;
>  	}

[Severity: Medium]
In the event that the first fork fails, is there a chance that all four
pipe file descriptors allocated earlier will be left open?

[ ... ]
> @@ -188,7 +187,7 @@ static bool sched_stress_test(bool is_ext)
>  		exit(0);
>  	} else if (rt_pid < 0) {
>  		perror("fork for RT task");
> -		ksft_exit_fail();
> +		goto out_kill_ext;
>  	}

[Severity: Medium]
Could jumping to out_kill_ext here cause the pipe file descriptors to be
leaked, since they aren't explicitly closed at that label?

[ ... ]
> @@ -199,45 +198,47 @@ static bool sched_stress_test(bool is_ext)
>  	 */
>  	close(ext_ready[1]);
>  	close(rt_ready[1]);
> -	wait_ready(ext_ready[0]);
> -	wait_ready(rt_ready[0]);
> +	if (!wait_ready(ext_ready[0]) || !wait_ready(rt_ready[0]))
> +		goto out_kill;

[Severity: Medium]
If wait_ready(ext_ready[0]) returns false, won't the short-circuit evaluation
skip calling wait_ready(rt_ready[0])? Since wait_ready() handles closing
the file descriptor, this could leave rt_ready[0] open.

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