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

On Thu, Apr 09, 2026 at 04:51:02PM +0000, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?

Right, maybe simply:

	if (argc) {
		evlist__start_workload(evlist);
		waitpid(evlist->workload.pid, NULL, 0);
	} else {
		while (!done)
			sleep(1);
	}

And SIG{INT,TERM,CHLD} should set done to true.

Thanks,
Namhyung

> 
> > +	sigprocmask(SIG_SETMASK, &oldmask, NULL);
> >  
> >  	if (reset) {
> >  		err = disable_sched_schedstat();
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1