> diff --git a/tools/testing/selftests/bpf/prog_tests/rhash.c
> b/tools/testing/selftests/bpf/prog_tests/rhash.c
> index 98bb66907b7f..a2ecd7e7905f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/rhash.c
> +++ b/tools/testing/selftests/bpf/prog_tests/rhash.c
> @@ -3,12 +3,17 @@
> #include <test_progs.h>
> #include <string.h>
> #include <stdio.h>
> +#include <pthread.h>
> +#include <stdatomic.h>
> #include "rhash.skel.h"
> #include "bpf_iter_bpf_rhash_map.skel.h"
> #include <linux/bpf.h>
> #include <linux/perf_event.h>
> #include <sys/syscall.h>
>
> +#define RHASH_STRESS_KEYS 4096
> +#define RHASH_STRESS_DURATION_NS (3ULL * 1000000000ULL)
^^^^
A subsystem pattern flags this as potentially concerning: the userspace
RHASH_STRESS_KEYS (4096) has to stay equal to stress_rhmap's
max_entries (RHASH_STRESS_MAX_ENTRIES, 4096 in progs/rhash.c:12) for
the test to keep its meaning. The point of the stress driver is to keep
the map at its capacity so that rehashing happens and a walk has enough
elements to exceed the budget. These are two independent literals in two
different files with no compile-time or run-time tie, so raising the map
size in the BPF object silently leaves the userspace threads churning
only the first 4096 keys.
The function fetches the authoritative value at line 113
(max_entries = bpf_map__max_entries(skel->maps.stress_rhmap);)
but uses it only for the printf() at line 144, so the key range could be
driven from max_entries instead of a duplicated literal.
Could the value that gates the assertion (RHASH_STRESS_MAX_ENTRIES) be
used both for max_entries and for the overrun threshold to avoid drift,
or could the userspace key range be driven from the fetched max_entries?
>
> static void rhash_run(const char *prog_name)
> {
[ ... ]
> +static void rhash_iter_stress(void)
> +{
> + struct rhash *skel = NULL;
> + struct rhash_stress_arg args[2];
> + LIBBPF_OPTS(bpf_test_run_opts, opts);
> + atomic_bool stop = false;
> + __u64 start;
> + __u32 max_entries;
> + pthread_t threads[2];
> + bool created[2] = {};
> + struct bpf_program *prog;
> + int map_fd, err, i;
> +
> + skel = rhash__open();
> + if (!ASSERT_OK_PTR(skel, "rhash__open stress"))
> + return;
> +
> + prog = bpf_object__find_program_by_name(skel->obj,
> + "test_rhash_iter_stress");
> + if (!ASSERT_OK_PTR(prog, "find stress program"))
> + goto cleanup;
> + bpf_program__set_autoload(prog, true);
> +
> + err = rhash__load(skel);
> + if (!ASSERT_OK(err, "stress skel_load"))
> + goto cleanup;
> +
> + map_fd = bpf_map__fd(skel->maps.stress_rhmap);
> + max_entries = bpf_map__max_entries(skel->maps.stress_rhmap);
> + for (i = 0; i < ARRAY_SIZE(threads); i++) {
> + args[i].map_fd = map_fd;
> + args[i].stop = &stop;
> + args[i].seed = i * 977;
> + err = pthread_create(&threads[i], NULL, rhash_stress_update,
> + &args[i]);
> + if (!ASSERT_OK(err, "pthread_create"))
> + goto stop_threads;
> + created[i] = true;
> + }
> +
> + start = get_time_ns();
> + while (get_time_ns() - start < RHASH_STRESS_DURATION_NS) {
> + err = bpf_prog_test_run_opts(bpf_program__fd(prog), &opts);
> + if (!ASSERT_OK(err, "stress prog run"))
> + break;
> + }
> +
> +stop_threads:
> + atomic_store(&stop, true);
> + for (i = 0; i < ARRAY_SIZE(threads); i++)
> + if (created[i])
> + pthread_join(threads[i], NULL);
> +
> + ASSERT_GT(skel->bss->stress_max_visits, 0, "stress callback visits");
> + ASSERT_EQ(skel->bss->stress_overruns, 0, "stress callback bound");
^^^^
Does the stop_threads label fall straight through into the result
assertions? If pthread_create() fails for i == 0 the BPF program is
never run, so skel->bss->stress_max_visits is still 0 and
ASSERT_GT(skel->bss->stress_max_visits, 0, ...) emits a second,
misleading failure (unexpected stress callback visits: actual 0 <=
expected 0) on top of the real pthread_create failure. The printf()
then reports "stress max callback visits: 0 (limit 4096)" as if a
measurement had been taken.
The same fall-through happens when bpf_prog_test_run_opts() fails on
the very first iteration (break out of the timing loop). Every other
error path in the function uses goto cleanup to skip the result checks.
Should the thread-creation path goto cleanup, or should the two result
assertions be guarded by a flag recording that at least one prog run
completed?
A subsystem pattern flags this as potentially concerning: the
load-bearing assertion is ASSERT_EQ(skel->bss->stress_overruns, 0),
i.e. no single walk returned more than max_entries. On a kernel without
the iteration bound fix, the walk still terminates naturally after
visiting roughly nelems (<= 4096) entries whenever the
rehash-during-walk race does not fire, so stress_overruns stays 0 and
the subtest reports PASS on the unfixed kernel.
Nothing in the test reports whether the race was actually exercised:
bpf_each_rhash_elem() runs under migrate_disable() + rcu_read_lock(), so
on a 1-vCPU CI VM the two updater threads cannot mutate the table during
a walk at all and the test can never detect the bug there, yet it still
passes.
The companion liveness check, ASSERT_GT(skel->bss->stress_max_visits,
0), is satisfied by a single visited element, and stress_max_visits
reads ~4096 both when the budget clamped a duplicate-heavy walk and when
a quiescent walk simply covered the whole table, so the printed number
cannot distinguish a real pass from a vacuous one either.
This isn't a bug, but would it be worth recording something that proves
the stress happened, such as a count of walks that consumed the full
max_entries budget, or a skip/warning when libbpf_num_possible_cpus()
is 1?
> + printf("stress max callback visits: %llu (limit %u)\n",
> + (unsigned long long)skel->bss->stress_max_visits, max_entries);
> +cleanup:
> + rhash__destroy(skel);
> +}
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/rhash.c
> b/tools/testing/selftests/bpf/progs/rhash.c
> index fc2dac3a719e..eca428db1fbc 100644
> --- a/tools/testing/selftests/bpf/progs/rhash.c
> +++ b/tools/testing/selftests/bpf/progs/rhash.c
[ ... ]
> +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;
> + if (visits > RHASH_STRESS_MAX_ENTRIES)
> + stress_overruns++;
> + return 0;
> +}
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33200467478
