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

On 4/24/26 10:05 PM, [email protected] 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 <[email protected]>
> 
> 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.