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

Mykyta Yatsenko
2026-04-25T21:29:31Z

On 4/24/26 10:05 PM, sashiko-bot@kernel.org wrote:
> 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?

Even calling rhtab_check_and_free_fields() after the copy_map_value()
does not guarantee that concurrent BPF program does not hold a pointer
to the value and uses special field like timer.

>
> [ ... ]
>> @@ -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?
>
yes, I'll address these rcu stall issues in v4.