Re: [PATCH v2 2/3] perf sched stats: Fix SIGCHLD race in schedstat_live()

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Unconditional use of sigsuspend() without a state flag creates a window where early signals are lost, leading to an indefinite hang.
- [High] Workload child process is orphaned if the tool is interrupted via SIGINT/SIGTERM.
--

commit f1c0b504081c33314d50fb6bf644c4e537e47385
Author: Swapnil Sapkal <swapnil.sapkal@amd.com>
Subject: perf sched stats: Fix SIGCHLD race in schedstat_live()

This commit attempts to fix a lost-wakeup race by blocking SIGCHLD, SIGINT,
and SIGTERM before starting the workload, and then using sigsuspend()
instead of pause() to wait for the workload to finish.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 3f448acd5046f..c716dd33a9c93 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -4702,11 +4703,17 @@ static int perf_sched__schedstat_live(struct perf_sched *sched,
>  	if (err < 0)
>  		goto out;
>  
> +	sigemptyset(&sig_mask);
> +	sigaddset(&sig_mask, SIGCHLD);
> +	sigaddset(&sig_mask, SIGINT);
> +	sigaddset(&sig_mask, SIGTERM);
> +	sigprocmask(SIG_BLOCK, &sig_mask, &oldmask);

Will this still hang if a signal arrives before sigprocmask() is called?

Because the registered signal handler is an empty dummy function without
a state flag, any signal arriving before sigprocmask() will be consumed
by the handler.

If a signal such as SIGINT or a premature SIGCHLD arrives during the earlier
setup phases (e.g., during /proc/schedstat parsing), the signal is
discarded and sigsuspend() will block indefinitely waiting for a signal
that has already arrived.

Should this handler set a flag (like a volatile sig_atomic_t) that is
checked after the sigprocmask(SIG_BLOCK) call but before sigsuspend()?

> +
>  	if (argc)
>  		evlist__start_workload(evlist);
>  
> -	/* wait for signal */
> -	pause();
> +	sigsuspend(&oldmask);
> +	sigprocmask(SIG_SETMASK, &oldmask, NULL);

Does the exit path leave the child workload running if interrupted?

While not introduced by this patch, if sigsuspend() returns due to an
interruption like SIGINT, the code proceeds to the cleanup path.

This path calls evlist__delete() but does not appear to send SIGTERM or
SIGKILL to the workload child process (evlist->workload.pid), nor does it
waitpid() for the child to terminate.

Could this leave the workload executing as an orphaned process in the
background, leaking resources?

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