[PATCH v3 0/3] perf: Fix SIGCHLD vs pause() race with short-lived workloads

Swapnil Sapkal posted 3 patches 1 month, 3 weeks ago
There is a newer version of this series
tools/perf/builtin-lock.c  | 17 +++++++++++++----
tools/perf/builtin-sched.c | 28 ++++++++++++++++++++--------
2 files changed, 33 insertions(+), 12 deletions(-)
[PATCH v3 0/3] perf: Fix SIGCHLD vs pause() race with short-lived workloads
Posted by Swapnil Sapkal 1 month, 3 weeks ago
Several perf subcommands (sched stats, lock contention) use the pattern
of forking a workload child, calling evlist__start_workload() to uncork
it, and then calling pause() to wait for a signal (typically SIGCHLD
when the child exits, or SIGINT/SIGTERM from the user).

This pattern has a race condition: if the workload is very short-lived,
the child can exit and deliver SIGCHLD in the window between
evlist__start_workload() and pause(). Since pause() only returns when a
signal is received *while the process is suspended*, and SIGCHLD has
already been delivered and handled by the empty sighandler(), pause()
blocks indefinitely.

The fix replaces pause() with a simpler approach:

 - When a workload is given (argc), use waitpid() to directly wait for
   the child process to exit.  This is race-free since waitpid()
   collects the child's status regardless of when it exited.

 - In system-wide mode (no workload), use 'while (!done) sleep(1)' to
   wait for SIGINT/SIGTERM.  The signal handler sets a
   'volatile sig_atomic_t done' flag, and sleep() is interrupted by
   signal delivery so the flag is checked promptly.

Three call sites are affected across two files:
  - perf_sched__schedstat_record() in builtin-sched.c
  - perf_sched__schedstat_live()   in builtin-sched.c
  - __cmd_contention()             in builtin-lock.c

The two pause() sites in builtin-kwork.c are NOT affected because they
do not register SIGCHLD or fork workload children; they only wait for
user-initiated SIGINT/SIGTERM.

Changes since v2:
  - Replaced sigsuspend()/sigprocmask() approach with the much simpler
    waitpid() + while(!done) sleep(1) pattern as suggested by
    Namhyung.  This eliminates all signal mask manipulation while
    still being race-free.

Changes since v1:
  - Moved sigprocmask() to after evlist__prepare_workload() so the
    forked child does not inherit a blocked SIGCHLD mask, which would
    break workloads relying on SIGCHLD (Sashiko review)
  - Block SIGINT and SIGTERM in addition to SIGCHLD to prevent an
    early Ctrl+C during setup from being consumed before sigsuspend().
  - Error paths before sigprocmask no longer need mask restoration
    since the mask is not yet modified at that point.
    (Sashiko review)

Swapnil Sapkal (3):
  perf sched stats: Fix SIGCHLD vs pause() race in schedstat_record()
  perf sched stats: Fix SIGCHLD vs pause() race in schedstat_live()
  perf lock contention: Fix SIGCHLD vs pause() race in __cmd_contention()

 tools/perf/builtin-lock.c  | 17 +++++++++++++----
 tools/perf/builtin-sched.c | 28 ++++++++++++++++++++--------
 2 files changed, 33 insertions(+), 12 deletions(-)

-- 
2.43.0