Re: [PATCH bpf-next v3 07/10] selftests/bpf: Add basic tests for resizable hash map

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Medium] The BPF test framework silently ignores test failures because it does not check the BPF program's return value (`opts.retval`) and incorrectly reuses the global `err` variable.
--

commit dc85c0ab999d049899213215cdd840f6c0a212f0
Author: Mykyta Yatsenko <yatsenko@meta.com>

selftests/bpf: Add basic tests for resizable hash map
    
Test basic map operations (lookup, update, delete) for BPF_MAP_TYPE_RHASH
including boundary conditions like duplicate key insertion and deletion of
nonexistent keys.

> diff --git a/tools/testing/selftests/bpf/prog_tests/rhash.c b/tools/testing/selftests/bpf/prog_tests/rhash.c
> new file mode 100644
> index 00000000000000..0f6eec4fe1fe1d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/rhash.c

[ ... ]

> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2026 Meta Platforms, Inc. and affiliates. */

[ ... ]

> +	err = rhash__load(skel);
> +	if (!ASSERT_OK(err, "skel_load"))
> +		goto cleanup;
> +
> +	err = bpf_prog_test_run_opts(bpf_program__fd(prog), &opts);
> +	if (!ASSERT_OK(err, "prog run"))
> +		goto cleanup;
> +
> +	if (!ASSERT_OK(skel->bss->err, "bss->err"))
> +		goto cleanup;

Does the test framework ignore the BPF program's return value here?

It seems opts.retval is not being checked.

> diff --git a/tools/testing/selftests/bpf/progs/rhash.c b/tools/testing/selftests/bpf/progs/rhash.c
> new file mode 100644
> index 00000000000000..9d066a22ce1865
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/rhash.c

[ ... ]

> @@ -0,0 +1,239 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2026 Meta Platforms, Inc. and affiliates. */

[ ... ]

> +SEC("syscall")
> +int test_rhash_update_delete(void *ctx)
> +{
> +	int key = 6;
> +	struct elem empty = {.val = 4, .arr = {0}};
> +	struct elem *e;
> +
> +	err = 1;
> +	e = bpf_map_lookup_elem(&rhmap, &key);
> +	if (e)
> +		return 1;
> +
> +	err = bpf_map_update_elem(&rhmap, &key, &empty, BPF_NOEXIST);
> +	if (err)
> +		return 2;
> +
> +	err = bpf_map_delete_elem(&rhmap, &key);
> +	if (err)
> +		return 3;
> +
> +	e = bpf_map_lookup_elem(&rhmap, &key);
> +	if (e)
> +		return 4;

Can this silently ignore test failures?

If this final lookup succeeds (meaning e is not NULL when it should be), the
BPF program exits early with return 4. However, the global err variable
was just set to 0 by the successful bpf_map_delete_elem() call above.

Because the userspace runner only checks ASSERT_OK(skel->bss->err) and ignores
opts.retval, won't this incorrectly mark a failed test as passed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260424-rhash-v3-0-d0fa0ce4379b@meta.com?part=7