Re: [PATCH v4 09/15] perf test: Add summary reporting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Deadlock and Use-After-Free due to async-signal-unsafe operations in signal-handling exit path.
--

commit 1fcb991e36afddc8078cf7b1052220591400728e
Author: Ian Rogers <irogers@google.com>

perf test: Add summary reporting

This commit introduces an automated global execution summary printed at the
end of the perf test run. It tracks passed, skipped, and failed test cases
using a global strbuf to accumulate the descriptions of failed tests.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -393,6 +403,15 @@ static int print_test_result(struct test_suite *t, int curr_suite, int curr_test
>  		break;
>  	case TEST_FAIL:
>  	default:
> +		summary_tests_failed++;
> +		if (test_suite__num_test_cases(t) > 1)
> +			strbuf_addf(&summary_failed_tests_buf, "  %3d.%1d: %s\n",
> +				    curr_suite + 1, curr_test_case + 1,
> +				    test_description(t, curr_test_case));
> +		else
> +			strbuf_addf(&summary_failed_tests_buf, "  %3d: %s\n",
> +				    curr_suite + 1,
> +				    test_description(t, curr_test_case));

[Severity: Medium]
Is it safe to use strbuf_addf() here, considering the signal handling in this
test harness?

Since strbuf_addf() can dynamically allocate memory, it is not
async-signal-safe. If a SIGINT or SIGTERM arrives while the allocator lock is
held, the signal handler cmd_test_sig_handler() will siglongjmp() out,
leaving the lock permanently held.

>  		color_fprintf(stderr, PERF_COLOR_RED, " FAILED!\n");
>  		break;
>  	}

[ ... ]

> @@ -963,6 +982,23 @@ static void cmd_test_sig_handler(int sig)
>  	siglongjmp(cmd_test_jmp_buf, sig);
>  }
>  
> +static void print_tests_summary(void)
> +{
> +	pr_info("\n=== Test Summary ===\n");

[ ... ]

> +	if (summary_tests_failed > 0) {
> +		color_fprintf(stderr, PERF_COLOR_RED, "Failed tests      : %u\n",
> +			      summary_tests_failed);
> +		pr_info("List of failed tests:\n");
> +		pr_info("%s", summary_failed_tests_buf.buf);
> +	} else {
> +		color_fprintf(stderr, PERF_COLOR_GREEN, "Failed tests      : 0\n");
> +	}
> +	strbuf_release(&summary_failed_tests_buf);
> +}

[ ... ]

> @@ -1075,6 +1114,7 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
>  		for (size_t x = 0; x < num_tests; x++)
>  			finish_test(child_tests, x, num_tests, width);
>  	}
> +	print_tests_summary();
>  	free(global_pfds);
>  	free(global_pfd_indices);
>  	global_pfds = NULL;

[Severity: Medium]
Could this cause a deadlock or a use-after-free if triggered by a signal?

If a signal interrupts strbuf_addf() during an allocation:

tools/perf/tests/builtin-test.c:print_test_result()
    strbuf_addf(&summary_failed_tests_buf, ...)
    /* interrupted during memory allocation */
    cmd_test_sig_handler()
    siglongjmp(cmd_test_jmp_buf, sig);

Execution resumes at the sigsetjmp() site, branches to the err_out label,
and then calls print_tests_summary().

Calling strbuf_release() from print_tests_summary() could attempt to acquire
the same memory allocator lock, causing a deadlock.

Additionally, if the interrupted allocation had just freed the old memory
block but not yet updated the buffer pointer, accessing it here would result
in reading and double-freeing freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531082235.940833-1-irogers@google.com?part=9