Re: [PATCH v2 bpf-next 3/3] selftests/bpf: Add a runtime test for private stack and stack arguments

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: broken compiler skip in serial_test_private_stack_runtime()
--

commit d5748c85c54dbc46700395b1926da29c28fcdef7
Author: Chen Pei <cp0613@linux.alibaba.com>

selftests/bpf: Add a runtime test for private stack and stack arguments

This commit adds a runtime and JIT disassembly selftest on 64-bit RISC-V for
BPF programs that combine an adaptive private stack with outgoing stack
arguments.

> diff --git a/tools/testing/selftests/bpf/prog_tests/private_stack_runtime.c b/tools/testing/selftests/bpf/prog_tests/private_stack_runtime.c
> new file mode 100644
> index 0000000000000..918c41d6b179c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/private_stack_runtime.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +#include "private_stack_runtime.h"
> +#include "private_stack_runtime.skel.h"
> +
> +void serial_test_private_stack_runtime(void)
> +{
> +#if defined(__riscv) && __riscv_xlen == 64
> +	LIBBPF_OPTS(bpf_test_run_opts, opts,
> +		    .data_in = &pkt_v4,
> +		    .data_size_in = sizeof(pkt_v4),
> +	);
> +	struct private_stack_runtime *skel;
> +	struct bpf_link *trace = NULL, *ops = NULL;
> +	cpu_set_t saved, single;
> +	int cpu, err, tested = 0;
> +
> +	/*
> +	 * Two independent halves. RUN_TESTS() validates the __jited patterns
> +	 * through the test loader, which builds its own object from the ELF
> +	 * bytes. The half below loads a separate skeleton to check the runtime
> +	 * values and the CPU the program reported, which disassembly patterns
> +	 * cannot express.
> +	 */
> +	RUN_TESTS(private_stack_runtime);
> +
> +	if (!ASSERT_OK(sched_getaffinity(0, sizeof(saved), &saved), "getaffinity"))
> +		return;
> +
> +	skel = private_stack_runtime__open();
> +	if (!ASSERT_OK_PTR(skel, "open"))
> +		return;
> +	if (!skel->rodata->has_stack_args) {
> +		printf("%s:SKIP: clang lacks __BPF_FEATURE_STACK_ARGUMENT\n", __func__);
> +		test__skip();
> +		goto out;
> +	}

[Severity: Medium]
In tools/testing/selftests/bpf/prog_tests/private_stack_runtime.c,
serial_test_private_stack_runtime() calls RUN_TESTS(private_stack_runtime)
before opening the skeleton and checking skel->rodata->has_stack_args.

When the BPF compiler does not define __BPF_FEATURE_STACK_ARGUMENT (such as
bpf-gcc or an older Clang), tools/testing/selftests/bpf/progs/private_stack_runtime.c
compiles run_args() as an empty stub and omits the __arch_riscv64 and
__jited() annotations on private_args and kernel_args without adding a
__skip() annotation:

tools/testing/selftests/bpf/progs/private_stack_runtime.c:
    #else
    const volatile bool has_stack_args = false;

    static __always_inline void run_args(struct private_stack_result *result)
    {
    }
    #endif

    SEC("raw_tp/sys_enter")
    #ifdef __BPF_FEATURE_STACK_ARGUMENT
    __arch_riscv64
    ...
    __jited("	ld	{{.*}}, 0x8(s0)")
    #endif

    int private_args(void *ctx)

When RUN_TESTS(private_stack_runtime) runs in that configuration,
parse_test_spec() and run_subtest() treat private_args, kernel_args, and
zero_stack as three valid subtests with no __jited() expectations, load all
three, and record 3 passing subtests before resetting env.subtest_state to
NULL.

Execution then reaches the !skel->rodata->has_stack_args check and calls
test__skip() outside any subtest, incrementing test_state->skip_cnt to 1 while
test_state->subtest_num is 3. Because skipped_cnt != subtests_cnt in
print_test_result(), doesn't this run three vacuous subtests and report the
test as OK (SKIP: 1/3) instead of skipping it when the compiler lacks
__BPF_FEATURE_STACK_ARGUMENT? Could the has_stack_args check be performed
before RUN_TESTS(private_stack_runtime), or should the programs be annotated
with __skip() when __BPF_FEATURE_STACK_ARGUMENT is undefined?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104100411.997-1-cp0613@linux.alibaba.com?part=3