Re: [PATCH v3 1/3] perf sched stats: Fix SIGCHLD vs pause() race in schedstat_record()

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Blocking waitpid() ignores the cancellation flag, causing a hang or premature exit.
- [Medium] Unconditional `done = 0;` assignment discards termination signals received during initialization.
--

commit 1c95f818d983ad8f7f5ed28e4a0036553b2d2758
Author: Swapnil Sapkal <swapnil.sapkal@amd.com>

perf sched stats: Fix SIGCHLD vs pause() race in schedstat_record()

This commit replaces pause() with waitpid() or a sleep() loop to fix an
indefinite hang if SIGCHLD is delivered before the process enters pause().
It also introduces a sig_atomic_t flag set by the signal handler.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 3f509cfdd58c4..cfd93bf11c2ee 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
[ ... ]
> @@ -3899,11 +3903,15 @@ static int perf_sched__schedstat_record(struct perf_sched *sched,
>  	if (err < 0)
>  		goto out;
>  
> -	if (argc)
> -		evlist__start_workload(evlist);
> +	done = 0;

Is it possible that this unconditional assignment discards termination signals
received during initialization?

Since the signal handlers are registered at the beginning of the function,
if a user sends SIGINT during the setup phase, the handler will set done to 1.
This assignment would then erase the early cancellation request, requiring the
user to send the signal again.

>  
> -	/* wait for signal */
> -	pause();
> +	if (argc) {
> +		evlist__start_workload(evlist);
> +		waitpid(evlist->workload.pid, NULL, 0);

Could this blocking waitpid() cause the tool to ignore the cancellation flag?

Because perf is compiled with _GNU_SOURCE, glibc's signal() uses the
SA_RESTART flag by default. This causes waitpid() to automatically restart
after the signal handler returns.

If the profiled workload ignores or delays handling SIGINT, waitpid() will
continue to block indefinitely and ignore the done flag.

Conversely, if SA_RESTART were disabled, waitpid() would return -1 (EINTR).
Since the return value is not checked, the program would exit prematurely
and leave the workload orphaned in the background.

Would it be safer to use waitpid() with WNOHANG inside a loop that checks
the done flag?

> +	} else {
> +		while (!done)
> +			sleep(1);
> +	}
>  
>  	if (reset) {
>  		err = disable_sched_schedstat();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260422050545.129448-1-swapnil.sapkal@amd.com?part=1