Re: [PATCH bpf-next v4 3/3] selftests/bpf: libarena: parallel test harness and spmc parallel selftest

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing file descriptor (FD) validation after manual program lookups.
--

commit f9cb27c914dcd0d4f6eca457c86b2aac266e34a4
Author: Emil Tsalapatis <emil@etsalapatis.com>

selftests/bpf: libarena: parallel test harness and spmc parallel selftest

Add a parallel test for the SPMC Lev-Chase workstealing queue. The queue
is built to be wait-free even when there are multiple consumers, and
the parallel selftest provides a signal on whether the queue behaves
correctly when stress tested.

> diff --git a/tools/testing/selftests/bpf/prog_tests/libarena.c b/tools/testing/selftests/bpf/prog_tests/libarena.c
> index 81bdb084c2715..aa2070f8be825 100644
> --- a/tools/testing/selftests/bpf/prog_tests/libarena.c
> +++ b/tools/testing/selftests/bpf/prog_tests/libarena.c
> @@ -27,6 +27,177 @@ static void run_libarena_test(struct libarena *skel, struct bpf_program *prog,
>  
>  }
>  
> +static void *run_libarena_parallel_prog(void *arg)
> +{
> +	struct bpf_program *prog = arg;
> +
> +	return (void *)(long)libarena_run_prog(bpf_program__fd(prog));

[Severity: Medium]
Should we validate the file descriptor returned by bpf_program__fd here?

The program is manually looked up via bpf_object__find_program_by_name
in run_libarena_parallel_test_workers before being passed to this thread
worker. When using manual lookups instead of auto-generated skeleton fields,
the program pointer could be valid while the underlying program is invalid,
resulting in a negative FD. 

An ASSERT_GE check for the file descriptor could prevent unexpected behavior.

> +}
> +
> +/* Max suffix is ceil((lg 2^32) / (lg 10)) + sizeof("__") = 10 + 2 = 12. */
> +#define MAX_PARTEST_SUFFIX (12)
> +#define MAX_PARTEST_NAME (1024)
> +#define MAX_PARTEST_PREFIX (MAX_PARTEST_NAME - MAX_PARTEST_SUFFIX)
> +
> +static int run_libarena_parallel_fini(struct libarena *skel, const char *name,
> +				      size_t prefixlen)
> +{
> +	char tdname[MAX_PARTEST_NAME];
> +	struct bpf_program *fini_prog;
> +	int ret;
> +
> +	ret = snprintf(tdname, sizeof(tdname), "%.*s__fini", (int)prefixlen, name);
> +	if (!ASSERT_LT(ret, sizeof(tdname), "partest fini name"))
> +		return -ENAMETOOLONG;
> +
> +	fini_prog = bpf_object__find_program_by_name(skel->obj, tdname);
> +	if (!ASSERT_TRUE(fini_prog, "partest fini prog"))
> +		return -ENOENT;
> +
> +	ret = libarena_run_prog(bpf_program__fd(fini_prog));

[Severity: Medium]
Does this missing file descriptor check risk passing a negative FD to
libarena_run_prog?

Similar to the above, we manually look up fini_prog. Checking the return
value of bpf_program__fd with ASSERT_GE before using it would align with
the BPF selftest guidelines for manual lookups.

> +	ASSERT_OK(ret, tdname);
> +
> +	return ret;
> +}

[ ... ]

> +static bool libarena_parallel_test_enabled(struct libarena *skel,
> +					   const char *prefix,
> +					   size_t prefixlen)
> +{
> +	struct bpf_program *prog;
> +	char progname[MAX_PARTEST_NAME];
> +	int ret;
> +
> +	ret = snprintf(progname, sizeof(progname), "%.*s__enabled", (int)prefixlen,
> +		       prefix);
> +	if (!ASSERT_LT(ret, sizeof(progname), "partest enabled name"))
> +		return false;
> +
> +	prog = bpf_object__find_program_by_name(skel->obj, progname);
> +	if (!prog)
> +		return true;
> +
> +	ret = libarena_run_prog(bpf_program__fd(prog));

[Severity: Medium]
Should we verify that the FD returned by bpf_program__fd is non-negative
before executing it?

Because progname is dynamically constructed and manually looked up, we do
not have the same FD validity guarantees as we do for auto-generated
skeleton fields.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605222020.5231-1-emil@etsalapatis.com?part=3