Re: [PATCH bpf v3 2/2] selftests/bpf: verify preemptible uprobes avoid private stack

> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_sleepable_stack.c b/tools/testing/selftests/bpf/prog_tests/uprobe_sleepable_stack.c
> new file mode 100644
> index 000000000000..a18970d2c584
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_sleepable_stack.c

A subsystem pattern flags this as potentially concerning: the BPF
selftests guide asks whether a new test file for a narrow variant of
behavior should instead be a new case in an existing more general test.

The behavior being tested - that the verifier disables private stack for
sleepable uprobes - is a verifier-level decision made in
bpf_enable_priv_stack() when prog->sleepable is set. The test suite
already has tools/testing/selftests/bpf/progs/verifier_private_stack.c
in the same directory, which covers private-stack selection
deterministically by asserting on JIT output patterns with __jited()
annotations.

This patch adds a 254-line, two-file runtime test that depends on thread
timing and CPU affinity. Adding a SEC("uprobe.s") case to the verifier
test with a 64-byte frame would pin the fix by asserting the absence of
the private-frame-pointer setup instruction, without needing threads or
timing dependencies.

The runtime test does exercise something the verifier test cannot -
actual concurrent same-CPU execution - so this is raised as a question:
would extending verifier_private_stack.c have been a simpler way to pin
the verifier change, or is the runtime concurrency test worth the added
complexity?

[ ... ]

> @@ -0,0 +1,165 @@

[ ... ]

> +void test_uprobe_sleepable_stack(void)
> +{
> +	struct uprobe_sleepable_stack *skel = NULL;
> +	cpu_set_t old_mask, mask;
> +	bool affinity_set = false;
> +	int cpu;
> +
> +#if !defined(__x86_64__) && !defined(__aarch64__) && !defined(__powerpc64__)
> +	test__skip();
> +	return;
> +#endif
> +	if (!env.jit_enabled) {
> +		test__skip();
> +		return;
> +	}

[ ... ]

> +	if (!skel->kconfig->CONFIG_PREEMPTION) {
> +		test__skip();
> +		goto cleanup;
> +	}

[ ... ]

> +	link_err = libbpf_get_error(link);
> +	if (link_err == -EOPNOTSUPP) {
> +		test__skip();
> +		return;
> +	}

This isn't a bug, but could the skip paths include diagnostics? All four
skip sites (unsupported architecture, JIT disabled, CONFIG_PREEMPTION
unset, and attach returning -EOPNOTSUPP) call bare test__skip() with no
message, so a skipped result gives the operator no way to tell which
prerequisite was missing.

The test_progs framework's test__skip() takes no arguments, so this
matches prevailing practice across the test suite. Is it worth noting
because the missing prerequisite that matters most - runtime preemption
actually being enabled - would be most helpful to communicate.

> diff --git a/tools/testing/selftests/bpf/progs/uprobe_sleepable_stack.c b/tools/testing/selftests/bpf/progs/uprobe_sleepable_stack.c

[ ... ]


---
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/32604492082