Re: [PATCH bpf-next v3 02/10] rhashtable: Add rhashtable_walk_enter_from()

sashiko-bot
2026-04-24T20:15:36Z

Sashiko AI review found 3 potential issue(s):
- [High] `rhashtable_walk_enter_from()` ignores `future_tbl` during resizes, causing stateless iterators to infinite loop.
- [High] `rhashtable_walk_enter_from()` fails to initialize `iter->list` for `rhltable`s, causing an uninitialized memory read and breaking iteration.
- [Medium] In `test_walk_enter_from()`, an error path jumps to `out` without setting the `err` variable, causing a silent test failure.
--

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

rhashtable: Add rhashtable_walk_enter_from()

This commit introduces rhashtable_walk_enter_from() to allow efficient
iteration resume by positioning the walker at the bucket containing the
given key. It also refactors the internal lookup functions and adds
test cases.

> diff --git a/lib/rhashtable.c b/lib/rhashtable.c
> --- a/lib/rhashtable.c
> +++ b/lib/rhashtable.c

[ ... ]

> @@ -692,6 +692,47 @@ void rhashtable_walk_enter(struct rhashtable *ht, struct rhashtable_iter *iter)
> }
> EXPORT_SYMBOL_GPL(rhashtable_walk_enter);
>
> +/**
> + * rhashtable_walk_enter_from - Initialise a walk starting at a key's bucket
[ ... ]
> +void rhashtable_walk_enter_from(struct rhashtable *ht,
> + struct rhashtable_iter *iter,
> + const void *key,
> + const struct rhashtable_params params)
> + __must_hold(RCU)
> +{
> + struct bucket_table *tbl;
> + struct rhash_head *he;
> +
> + rhashtable_walk_enter(ht, iter);
> +
> + if (!key)
> + return;
> +
> + tbl = iter->walker.tbl;
> + if (!tbl)
> + return;
> +
> + he = __rhashtable_lookup_one(ht, tbl, key, params,
> + RHT_LOOKUP_NORMAL);
> + if (!he)
> + return;

What happens if the table is currently resizing and the key has already been
moved to the future table?

If __rhashtable_lookup_one() fails to find the key in the current table, it
leaves the iterator positioned at bucket 0.

For stateless iterators like BPF's get_next_key, could this cause an
infinite loop where it constantly resets to the beginning of the old table
until the resize completes?

> +
> + iter->slot = rht_key_hashfn(ht, tbl, key, params);
> + iter->p = he;

Does this leave iter->list uninitialized for rhlist tables?

rhashtable_walk_enter() does not initialize iter->list. When
rhashtable_walk_start_check() is called for an rhlist table, it validates
the element using:

lib/rhashtable.c:rhashtable_walk_start_check() {
...
} else if (iter->p && rhlist) {
...
if (list == iter->list) {
iter->p = p;
iter->skip = skip;
goto found;
}
...
}

Will this uninitialized read of iter->list cause the validation to fail
and silently fall back to the beginning of the bucket?

> +}
> +EXPORT_SYMBOL_GPL(rhashtable_walk_enter_from);

[ ... ]

> diff --git a/lib/test_rhashtable.c b/lib/test_rhashtable.c
> --- a/lib/test_rhashtable.c
> +++ b/lib/test_rhashtable.c

[ ... ]

> @@ -679,6 +680,133 @@ static int threadfunc(void *data)
> return err;
> }
>
> +static int __init test_walk_enter_from(void)
> +{

[ ... ]

> + /* Count the number of elements after this one */
> + do {
> + obj = rhashtable_walk_next(&iter);
> + count++;
> + } while (obj && (!IS_ERR(obj) || (PTR_ERR(obj) == -EAGAIN)));
> +
> + rhashtable_walk_stop(&iter);
> + rhashtable_walk_exit(&iter);
> + if (count > 4) {
> + pr_warn("walk_enter_from: walked %d elements instead of max 4\n", count);
> + goto out;
> + }

Does this error path need to set the err variable?

Right now it jumps to out and returns 0, which might cause the test suite
to silently miss the failure.

> + /*
> + * Collect the lengths of every walked path,
> + * we should have each length walked one time
> + */
> + len_freq[count - 1]++;
> + }

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