Re: [PATCH bpf-next v3 4/4] selftests/bpf: Test rhtab special-field combinations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing file descriptor validation after manual program lookup.
- [Low] Multi-line comments violate the modern BPF kernel comment style by placing text on the opening line.
--

commit c8d6d3baf7be1c36675cff2687886747c8f66dba
Author: Yuan Chen <chenyuan@kylinos.cn>

selftests/bpf: Test rhtab special-field combinations
    
This commit introduces tests for BPF_MAP_TYPE_RHASH with different special
field combinations like spin locks, timers, and kptrs. It verifies element
recycling behavior and ensures references are correctly maintained.

> diff --git a/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c b/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c
> new file mode 100644
> index 0000000000000..93e8cbacca656
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c
> @@ -0,0 +1,337 @@
[ ... ]
> +/* Userspace view of the lkmap value. The BPF side owns the real layout;
> + * the spin lock and the kptr are special fields that value copies skip,
> + * so only the plain bytes actually matter here.
> + */

[Severity: Low]
This isn't a bug, but does this violate the modern kernel multi-line comment
style?

The preferred BPF style requires the opening /* to be on its own line. There
are several multi-line comments in this patch (both here and in
tools/testing/selftests/bpf/progs/rhtab_fields.c) that put text on the same
line as the opening /*.

[ ... ]
> +static int run_prog(struct rhtab_fields *skel, const char *name, int *retval)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, topts);
> +	struct bpf_program *prog;
> +	int err;
> +
> +	prog = bpf_object__find_program_by_name(skel->obj, name);
> +	if (!ASSERT_OK_PTR(prog, name))
> +		return -1;
> +	err = bpf_prog_test_run_opts(bpf_program__fd(prog), &topts);

[Severity: Medium]
When manually looking up a program with bpf_object__find_program_by_name(),
bpf_program__fd() can return a negative value if the program is not valid.

Could we pass a negative file descriptor into bpf_prog_test_run_opts() here
if that happens? Should we validate the file descriptor with ASSERT_GE()
before using it?

> +	if (!ASSERT_OK(err, name))
> +		return -1;
> +	if (retval)
> +		*retval = topts.retval;
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901062845.1379760-1-chenyuan_fl@163.com?part=4