Re: [PATCH bpf-next v3 05/10] bpf: Allow timers, workqueues and task_work in resizable hashtab

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Unbounded iteration over the entire hash table inside a single RCU read-side critical section in `rhtab_map_free_internal_structs`.
- [High] Race condition in `rhtab_map_update_existing` allows concurrently initialized special fields (timers, kptrs) to survive map updates.
--

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

bpf: Allow timers, workqueues and task_work in resizable hashtab

This commit allows resizable hashtab maps to support deferred callbacks by
enabling timers, workqueues, and task_work field types. It also wires up
bpf_obj_free_fields during element deletion and map teardown to properly
clean up BTF record fields.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 000caa2c7f4cd9..ba03797b896020 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
[ ... ]
> @@ -2945,11 +2944,15 @@ static int rhtab_map_lookup_and_delete_elem(struct bpf_map *map, void *key, void
>  static long rhtab_map_update_existing(struct bpf_map *map, struct rhtab_elem *elem, void *value,
>  				      u64 map_flags)
>  {
[ ... ]
> +	/* Free special fields, and reuse memory similar to arraymap */
> +	rhtab_check_and_free_fields(rhtab, elem);
> +
>  	if (map_flags & BPF_F_LOCK)
>  		copy_map_value_locked(map, old_val, value, false);
>  	else
>  		copy_map_value(map, old_val, value);

Does calling rhtab_check_and_free_fields() before copy_map_value()
create a race condition with concurrent BPF programs?

If a concurrent program acquires the spinlock and initializes a new
timer or kptr right after rhtab_check_and_free_fields() finishes,
would those newly initialized special fields survive the update?
The subsequent copy_map_value() call intentionally skips special fields.

Should rhtab_check_and_free_fields() be called after the copy completes
to ensure no concurrent modifications survive the element update?

[ ... ]
> @@ -3033,6 +3037,25 @@ static struct rhtab_elem *rhtab_iter_next(struct rhashtable_iter *iter)
>  
>  static void rhtab_map_free_internal_structs(struct bpf_map *map)
>  {
> +	struct bpf_rhtab *rhtab = container_of(map, struct bpf_rhtab, map);
> +	struct rhashtable_iter iter;
> +	struct rhtab_elem *elem;
> +
> +	if (!bpf_map_has_internal_structs(map))
> +		return;
> +
> +	/*
> +	 * An element can be processed twice if rhashtable resized concurrently.
> +	 * Special structs freeing handles duplicate cancel_and_free.
> +	 */
> +	rhashtable_walk_enter(&rhtab->ht, &iter);
> +	rhashtable_walk_start(&iter);
> +
> +	for (elem = rhtab_iter_next(&iter); elem; elem = rhtab_iter_next(&iter))
> +		bpf_map_free_internal_structs(map, rhtab_elem_value(elem, map->key_size));
> +
> +	rhashtable_walk_stop(&iter);
> +	rhashtable_walk_exit(&iter);
>  }

Can this loop trigger RCU CPU stall warnings for large hash maps?

The function rhashtable_walk_start() explicitly acquires the RCU read lock,
meaning this entire iteration over potentially millions of elements occurs
inside a single RCU read-side critical section.

If the map contains many elements, looping without yielding could block
RCU grace periods for an extended time.

Would it be better to periodically drop the lock and yield, similar to
how other map structures handle large iterations?

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