Re: [PATCH v2 3/3] perf lock contention: Fix SIGCHLD race in __cmd_contention()

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [High] Unconditional use of `sigsuspend()` without a state flag fails to fix the race condition and causes indefinite hangs.
--

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

perf lock contention: Fix SIGCHLD race in __cmd_contention()

This commit replaces pause() with sigsuspend() to prevent a race condition
where a short-lived workload exits and delivers SIGCHLD before the wait
begins.

> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index e8962c985d34a..2da689de134a4 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c

[ ... ]

> @@ -2123,12 +2124,25 @@ static int __cmd_contention(int argc, const char **argv)
>  	}
>  
>  	if (use_bpf) {
> +		/*
> +		 * Block all handled signals after evlist__prepare_workload()
> +		 * so the forked child does not inherit a modified signal
> +		 * mask, then use sigsuspend() to atomically unblock and
> +		 * wait. This prevents a race where a short-lived workload
> +		 * exits and delivers SIGCHLD before we are ready to wait.
> +		 */
> +		sigemptyset(&sig_mask);
> +		sigaddset(&sig_mask, SIGCHLD);
> +		sigaddset(&sig_mask, SIGINT);
> +		sigaddset(&sig_mask, SIGTERM);
> +		sigprocmask(SIG_BLOCK, &sig_mask, &oldmask);
> +
>  		lock_contention_start();
>  		if (argc)
>  			evlist__start_workload(con.evlist);
>  
> -		/* wait for signal */
> -		pause();
> +		sigsuspend(&oldmask);

If SIGINT or SIGCHLD is delivered before sigprocmask() blocks them, won't it
be consumed by sighandler without setting any checked state, causing
sigsuspend() to hang indefinitely?

The signals are registered earlier in the function, leaving a window during
evlist__prepare_workload() and lock_contention_prepare() where they are
unblocked.

Should there be a volatile sig_atomic_t flag set by sighandler that is
checked in a loop, for example:

while (!done)
        sigsuspend(&oldmask);

This would avoid missing signals that arrive before the block and ensure
sigsuspend() doesn't hang.

> +		sigprocmask(SIG_SETMASK, &oldmask, NULL);
>  
>  		lock_contention_stop();
>  		lock_contention_read(&con);

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