> diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
> index 434065215d127..8748e2f4a915d 100644
> --- a/tools/testing/selftests/x86/Makefile
> +++ b/tools/testing/selftests/x86/Makefile
> @@ -19,7 +19,8 @@ TARGETS_C_32BIT_ONLY := entry_from_vm86 test_syscall_vdso unwind_vdso \
> test_FCMOV test_FCOMI test_FISTTP \
> vdso_restorer
> TARGETS_C_64BIT_ONLY := fsgsbase sysret_rip syscall_numbering \
> - corrupt_xstate_header amx lam test_shadow_stack avx apx
> + corrupt_xstate_header amx lam test_shadow_stack avx apx \
> + test_shadow_stack_prctl
> # Some selftests require 32bit support enabled also on 64bit systems
> TARGETS_C_32BIT_NEEDED := ldt_gdt ptrace_syscall
>
> @@ -138,3 +139,5 @@ $(OUTPUT)/avx_64: CFLAGS += -mno-avx -mno-avx512f
> $(OUTPUT)/amx_64: EXTRA_FILES += xstate.c
> $(OUTPUT)/avx_64: EXTRA_FILES += xstate.c
> $(OUTPUT)/apx_64: EXTRA_FILES += xstate.c
> +$(OUTPUT)/test_shadow_stack_64: EXTRA_FILES += shadow_stack.c
> +$(OUTPUT)/test_shadow_stack_prctl_64: EXTRA_FILES += shadow_stack.c
Does incremental make produce stale binaries here? shadow_stack.c is added
via EXTRA_FILES but isn't made a prerequisite of either binary. The pattern
rule at Makefile:44 is:
$(BINARIES_64): $(OUTPUT)/%_64: %.c helpers.h
so the prerequisites of test_shadow_stack_64 remain 'test_shadow_stack.c
helpers.h', and shadow_stack.c (which holds most of the test body after this
commit) isn't in that list. After editing shadow_stack.c, make reports
nothing to be done and leaves the previously-linked binary in place.
Lines 112-120 define an extra-files helper specifically for this: its
comment states 'Note: the only way to get extra dependencies is to
override the target (by leading 'test_XXX.c' with 'override test_XXX :')'.
The existing amx/avx/apx EXTRA_FILES lines (immediately above these new
ones) have the same omission, so this follows local precedent. What makes
it more visible here is that this commit moves 1160 of test_shadow_stack.c's
1167 lines into shadow_stack.c, so essentially the entire test body is now
in a file that isn't a build dependency of either binary.
> diff --git a/tools/testing/selftests/x86/shadow_stack.c b/tools/testing/selftests/x86/shadow_stack.c
> new file mode 100644
> index 0000000000000..a375c4ff7a6ac
> --- /dev/null
> +++ b/tools/testing/selftests/x86/shadow_stack.c
[ ... ]
> +#define SHADOW_STACK_ENABLE(which_test) \
> + ((which_test) == SHADOW_STACK_TEST_ARCH_PRCTL ? \
> + ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK) : \
> + PRCTL(PR_SET_SHADOW_STACK_STATUS, PR_SHADOW_STACK_ENABLE, 0, 0, 0))
> +
> +#define SHADOW_STACK_ENABLE_WRITE(which_test) \
> + ((which_test) == SHADOW_STACK_TEST_ARCH_PRCTL ? \
> + ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_WRSS) : \
> + PRCTL(PR_SET_SHADOW_STACK_STATUS, \
> + PR_SHADOW_STACK_ENABLE|PR_SHADOW_STACK_WRITE, 0, 0, 0))
Are the SHADOW_STACK_ENABLE branches equivalent? Looking at
arch_set_shadow_stack_status() in arch/x86/kernel/shstk.c (from earlier in
this series), the prctl path with only PR_SHADOW_STACK_ENABLE falls through
to:
return shstk_prctl(t, ARCH_SHSTK_DISABLE, ARCH_SHSTK_WRSS);
so it clears WRSS, while ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK)
leaves WRSS untouched. shadow_stack_run_tests() enables WRSS once at line
1108 via SHADOW_STACK_ENABLE_WRITE(), then re-enables shadow stack with
SHADOW_STACK_ENABLE() inside test_uretprobe() (line 881) and
test_uprobe_call() (line 960).
In prctl mode those two calls silently drop WRSS. Nothing after line 881
currently uses write_shstk(), so no test fails today, but any future
wrss-using test added after the uprobe tests would only fail in the prctl
binary.
[ ... ]
> diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
> index 3d6ca33edba42..d89c04623001c 100644
> --- a/tools/testing/selftests/x86/test_shadow_stack.c
> +++ b/tools/testing/selftests/x86/test_shadow_stack.c
[ ... ]
> @@ -1,1174 +1,21 @@
> // SPDX-License-Identifier: GPL-2.0
> +
> /*
> * This program test's basic kernel shadow stack support. It enables shadow
> * stack manual via the arch_prctl(), instead of relying on glibc. It's
> * Makefile doesn't compile with shadow stack support, so it doesn't rely on
> * any particular glibc. As a result it can't do any operations that require
> * special glibc shadow stack support (longjmp(), swapcontext(), etc). Just
> - * stick to the basics and hope the compiler doesn't do anything strange.
> + * stick to the basics and hope the compiler doesn't do anything strange. It
> + * uses the x86 specific interface for ARCH_PCTL, whereas
> + * test_shadow_stack_prctl.c uses the generic PRCTL interface mixed with x86
> + * specific code.
> */
This isn't a bug, but there's a typo in the new sentence: 'ARCH_PCTL' should
be 'arch_prctl()' (the first paragraph already spells it correctly).
[ ... ]
> +int main(int argc, char *argv[])
> +{
> + return shadow_stack_run_tests(SHADOW_STACK_TEST_ARCH_PRCTL);
> +}
A subsystem pattern flags these test binaries for not using the kselftest
TAP wrappers: main() returns the helper's int status directly instead of
calling ksft_exit_*(), and shadow_stack_run_tests() reports results with
hand-rolled printf("[OK]")/printf("[FAIL]") text rather than
ksft_test_result_*().
Without TAP ok/not ok lines, a runner parsing results cannot distinguish
a skip from a failure: shadow_stack_run_tests() returns 1 both for a
genuine failure and for the "[SKIP] Could not enable Shadow stack" path
(shadow_stack.c:1131), so a kernel or CPU without the new generic prctl
support surfaces as a failing exit status rather than KSFT_SKIP.
The ad hoc printing is pre-existing (the old test_shadow_stack.c main()
had the same bare return and printf style), and every other test in
tools/testing/selftests/x86/ follows this convention rather than using
kselftest.h. Converting to TAP would be a rework of the whole x86
shadow-stack suite. Is the exit-status conflation for the new prctl
binary's skip case worth addressing separately?
The same pattern appears in test_shadow_stack_prctl.c below.
[ ... ]
The parameterized shadow_stack_run_tests() now takes 'enum shadow_stack_test'
and shadow_stack.h even declares SHADOW_STACK_TEST_COUNT, yet this main()
hardcodes SHADOW_STACK_TEST_ARCH_PRCTL and a second, near-identical binary
(test_shadow_stack_prctl.c) passes the other variant. Could a loop here:
for (i = 0; i < SHADOW_STACK_TEST_COUNT; i++)
ret |= shadow_stack_run_tests(i);
exercise both interfaces from a single test, or is there a reason the two
must run as separate processes?
> diff --git a/tools/testing/selftests/x86/test_shadow_stack_prctl.c b/tools/testing/selftests/x86/test_shadow_stack_prctl.c
> new file mode 100644
> index 0000000000000..4641a3b1ca036
> --- /dev/null
> +++ b/tools/testing/selftests/x86/test_shadow_stack_prctl.c
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Same as test_shadow_stack.c but uses the PRCTL interface
> + * instead of ARCH_PRCTL.
> + */
> +#include "shadow_stack.h"
> +
> +int main(int argc, char *argv[])
> +{
> + return shadow_stack_run_tests(SHADOW_STACK_TEST_PRCTL);
> +}
The TAP reporting concern from test_shadow_stack.c applies here too: the
new prctl binary returns 1 both for genuine failures and for the
"[SKIP] Could not enable Shadow stack" path, so a missing prerequisite
(kernel or CPU without PR_SET_SHADOW_STACK_STATUS support) surfaces as a
non-zero exit status rather than KSFT_SKIP.
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31820235607
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.