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

[sashiko-bot@kernel.org]

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