Re: [PATCH bpf v2 2/2] selftests/bpf: add RHASH iteration stress test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing Error Check / Integer Signedness Bug in BPF map iteration
- [Medium] Flawed test logic caps maximum iterations at working set size, causing flaky failures on valid duplicate visits
--

commit a9ec4aae45f967791f38abbe6d019f58ee4a93fe
Author: Hui Su <sh_def@163.com>

selftests/bpf: add RHASH iteration stress test

This commit adds a stress test for bpf_for_each_map_elem() on
BPF_MAP_TYPE_RHASH. It concurrently updates and deletes entries from two
threads while repeatedly invoking the BPF callback to verify that the map
iterator handles concurrent rehashing correctly.

Link: https://lore.kernel.org/bpf/20260828183326.3330530-2-sh_def@163.com/

> diff --git a/tools/testing/selftests/bpf/progs/rhash.c b/tools/testing/selftests/bpf/progs/rhash.c
> --- a/tools/testing/selftests/bpf/progs/rhash.c
> +++ b/tools/testing/selftests/bpf/progs/rhash.c
[ ... ]
> @@ -246,3 +267,23 @@ int test_rhash_delete_nonexistent(void *ctx)
>  	err = 0;
>  	return 0;
>  }
> +
> +SEC("syscall")
> +int test_rhash_iter_stress(void *ctx)
> +{
> +	/*
> +	 * Concurrent rehash may produce duplicate visits. Check that the
> +	 * helper still gives one walk a finite callback bound; no snapshot
> +	 * or unique-visit guarantee is expected here.
> +	 */
> +	long visits;
> +
> +	visits = bpf_for_each_map_elem(&stress_rhmap, stress_iter_cb, NULL, 0);
> +	if (visits > stress_max_visits)
> +		stress_max_visits = visits;

[Severity: Medium]
What happens if bpf_for_each_map_elem() encounters an error and returns a
negative error code like -EINVAL?

Because visits is a signed long and stress_max_visits is a volatile __u64,
the C integer promotion rules will convert the negative error code into a
large positive unsigned value (e.g., ~1.84e19).

Does this missing error check regression corrupt the stress_max_visits state
and mask the failure as a massive success count?

> +	if (visits > stress_working_set)
> +		stress_overruns++;

[Severity: Medium]
Could this condition cause sporadic test failures by incorrectly flagging
legitimate runtime behavior as an overrun?

The commit message notes that concurrent rehashing may cause duplicate visits,
which are permitted by RHASH's best-effort iteration semantics. If a walk
occurs when the map is fully populated with 4096 elements and encounters even
one permitted duplicate visit (e.g., due to a rehash moving a visited element
into a remaining bucket), the callback count will be 4097.

Since 4097 strictly exceeds stress_working_set (4096), stress_overruns will
increment, which will then cause the userspace assertion
ASSERT_EQ(skel->bss->stress_overruns, 0) in rhash_iter_stress() to fail.

Should there be headroom allowed above the working set size to account for
these permitted duplicate visits?

> +	if (visits == stress_working_set)
> +		stress_full_walks++;
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831140132.117755-1-sh_def@163.com?part=2