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

Namhyung Kim
2026-04-22T21:21:29Z

Hello,

On Wed, Apr 22, 2026 at 05:29:52AM +0000, sashiko-bot@kernel.org wrote:
> 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.

Right, as it's initialized to 0 you don't need to set it here.

>
> >
> > - /* 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?

Right, something like this? (not tested)

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

Thanks,
Namhyung

>
> > + } 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