Re: [PATCH v3 5/5] selftests/x86: add generic prctl shadow stack test

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The global `_which_test` variable used by `segv_gp_handler` is only initialized inside `test_32bit()`, creating an implicit execution dependency.
--

--- Patch [5]: [PATCH v3 5/5] selftests/x86: add generic prctl shadow stack test ---
commit 4df1cdb98415fac5314af3be03f1bd5e57dfbd7c
Author: Bill Roberts <[email protected]>

selftests/x86: add generic prctl shadow stack test

This commit refactors the existing shadow stack selftests to allow running
the suite using both arch_prctl and the generic prctl interfaces. It moves
the core logic into a shared file and introduces a new test executable for
the generic prctl.

> diff --git a/tools/testing/selftests/x86/shadow_stack.c b/tools/testing/selftests/x86/shadow_stack.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/testing/selftests/x86/shadow_stack.c
[ ... ]
> +sigjmp_buf jmp_buffer;
> +enum shadow_stack_test _which_test;
> +void segv_gp_handler(int signum, siginfo_t *si, void *uc)
> +{
> +	segv_triggered = true;
> +
> +	/*
> +	 * To work with old glibc, this can't rely on siglongjmp working with
> +	 * shadow stack enabled, so disable shadow stack before siglongjmp().
> +	 */
> +	SHADOW_STACK_DISABLE(_which_test);
> +	siglongjmp(jmp_buffer, -1);
> +}
[ ... ]
> +int test_32bit(enum shadow_stack_test which_test)
> +{
> +	struct sigaction sa = {};
> +	struct sigaction *sa32;
> +
> +	_which_test = which_test;
[ ... ]
> +int shadow_stack_run_tests(enum shadow_stack_test which_test)
> +{
> +	int ret = 0;
> +

[Severity: Medium]
Could this create a test execution order dependency?

The global variable _which_test is used by segv_gp_handler to multiplex
between the arch_prctl and prctl APIs before calling siglongjmp(). However,
_which_test is only explicitly initialized when test_32bit() executes.

Since segv_gp_handler is also used by test_uretprobe() and test_uprobe_call(),
if the test execution order changes or if test_32bit() gets skipped,
_which_test will remain uninitialized (defaulting to 0, which corresponds
to SHADOW_STACK_TEST_ARCH_PRCTL). This could cause the generic prctl test
binary to incorrectly use the arch_prctl APIs in its signal handler.

Should _which_test be initialized here at the entry of
shadow_stack_run_tests() instead of inside test_32bit() to ensure it is
always cleanly set for all tests that rely on this signal handler?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5