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

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Medium] The patch leaves a race window where early signals (like SIGINT) are lost, causing sigsuspend() to hang indefinitely.
--

commit 82e42f3b8c8062e3b83019c07f66b6e7199c4104
Author: Swapnil Sapkal <swapnil.sapkal@amd.com>

perf sched stats: Fix SIGCHLD race in schedstat_record()

This commit attempts to fix a SIGCHLD race in perf sched stats record by
replacing pause() with sigsuspend(). It blocks SIGCHLD, SIGINT, and SIGTERM
before starting the workload to ensure no signal is lost if the child exits
quickly.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c

[ ... ]

> @@ -3899,11 +3900,25 @@ static int perf_sched__schedstat_record(struct perf_sched *sched,
>  	if (err < 0)
>  		goto out;
>  
> +	/*
> +	 * Block all handled signals so that a short-lived workload exiting
> +	 * (SIGCHLD) or an early Ctrl+C (SIGINT/SIGTERM) during the remaining
> +	 * setup cannot be delivered before we are ready to wait. sigsuspend()
> +	 * below will atomically unblock them. This is done after
> +	 * evlist__prepare_workload() so the forked child does not inherit a
> +	 * modified signal mask.
> +	 */
> +	sigemptyset(&sig_mask);
> +	sigaddset(&sig_mask, SIGCHLD);
> +	sigaddset(&sig_mask, SIGINT);
> +	sigaddset(&sig_mask, SIGTERM);
> +	sigprocmask(SIG_BLOCK, &sig_mask, &oldmask);

Does this leave a race window where early signals like SIGINT can be lost,
causing sigsuspend() to hang indefinitely?

If a signal like SIGINT or SIGTERM is delivered before this sigprocmask()
call, the existing sighandler will consume it without setting any state flag
that gets checked here.

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

When execution subsequently reaches sigsuspend(), wouldn't it block
indefinitely waiting for an event that was already processed?

To robustly resolve the TOCTOU race without leaking a modified signal mask
to the child, should the signal handler set a volatile sig_atomic_t flag,
and sigsuspend() be called conditionally based on that flag?

> +	sigprocmask(SIG_SETMASK, &oldmask, NULL);
>  
>  	if (reset) {
>  		err = disable_sched_schedstat();

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