Re: [PATCH RFC bpf-next v2 08/18] bpf: Implement iterator APIs for resizable hashtab

> On Aug 14, 2026, at 9:49 PM, Andrii Nakryiko <[email protected]> wrote:
> 
> Is it really a good use of tokens for sashiko to review old patches
> that have either landed or were significantly changed since (and
> probably got a bunch of new revisions sent), or just plain forgotten
> by now?
> 
> Did something regress recently? I'm not sure this has happened before
> until recently...

Ooops, it’s probably a recent regression I had which blocked sashiko reviews for few hours,
so I had to restart some patch(set) semi-manually. Likely this one accidentally got into this group.
Should be a one time off though.

Thanks for reporting!


> 
> -- Andrii
> 
>> On Thu, Aug 13, 2026 at 4:59 PM <[email protected]> wrote:
>> 
>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>> - [High] Uninitialized memory read in `last_key` when `seq_show` overflows.
>> - [High] `rhashtable_walk_enter_from` resets iterator on deleted cursor key, causing O(N^2) complexity and infinite loops.
>> - [Medium] Missing __GFP_NOWARN flag in user-controlled kmalloc for map->key_size can lead to OOM splats.
>> --
>> 
>> --- Patch [8]: [PATCH RFC bpf-next v2 08/18] bpf: Implement iterator APIs for resizable hashtab ---
>> commit 226ce1988038cf4876ff0a72b5ae8e5ebc263151
>> Author: Mykyta Yatsenko <[email protected]>
>> 
>> bpf: Implement iterator APIs for resizable hashtab
>> 
>> Wire up seq_file BPF iterator for BPF_MAP_TYPE_RHASH so that
>> bpf_iter and bpftool map dump work with resizable hash maps.
>> 
>>> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
>>> index 4611671bf8e74..5ca4d26e70178 100644
>>> --- a/kernel/bpf/hashtab.c
>>> +++ b/kernel/bpf/hashtab.c
>> 
>> [ ... ]
>> 
>>> @@ -3205,36 +3218,113 @@ struct bpf_iter_seq_rhash_map_info {
>>>      struct bpf_map *map;
>>>      struct bpf_rhtab *rhtab;
>>>      struct rhashtable_iter iter;
>>> -     u32 skip_elems;
>>> +     void *last_key;
>>>      bool iter_active;
>>> };
>>> 
>>> static void *bpf_rhash_map_seq_start(struct seq_file *seq, loff_t *pos)
>>> {
>>> -     return NULL;
>>> +     struct bpf_iter_seq_rhash_map_info *info = seq->private;
>>> +     struct rhtab_elem *elem;
>>> +     void *key = *pos > 0 ? info->last_key : NULL;
>>> +
>>> +     scoped_guard(rcu) {
>>> +             rhashtable_walk_enter_from(&info->rhtab->ht, &info->iter,
>>> +                                        key, info->rhtab->params);
>> 
>> [Severity: High]
>> Will this silently restart iteration from bucket 0 if the cursor key is deleted?
>> 
>> rhashtable_walk_enter_from() looks up the provided cursor key to resume
>> iteration. If the key is missing, it returns early and leaves the iterator at
>> bucket 0 (iter->slot = 0).
>> 
>> In __rhtab_map_lookup_and_delete_batch(), the batch operation deletes elements
>> and returns the last deleted key to userspace. On the next batch syscall, the
>> user passes this deleted key back as the cursor. Since the key no longer
>> exists, rhashtable_walk_enter_from() resets the iterator to 0.
>> 
>> The inner loop then executes while (total < max_count) holding rcu_read_lock(),
>> rescanning all previously emptied buckets from the beginning of the table.
>> 
>> For lookup_batch and seq_file iterators, does this yield massive duplicate
>> outputs to userspace and enormous RCU stall times?
>> 
>>> +             rhashtable_walk_start(&info->iter);
>>> +     }
>>> +     info->iter_active = true;
>>> +
>>> +     elem = rhtab_iter_next(&info->iter);
>>> +     if (!elem)
>>> +             return NULL;
>>> +     /*
>>> +      * if *pos is not 0, previously iteration failed on this elem,
>>> +      * so we are restarting it. That's why no need to increment *pos.
>>> +      */
>>> +     if (*pos == 0)
>>> +             ++*pos;
>> 
>> [Severity: High]
>> Does this pass uninitialized memory as a hash key if seq_show() output
>> overflows?
>> 
>> When reading a BPF iterator, seq_read_iter() calls bpf_rhash_map_seq_start(),
>> which increments *pos from 0 to 1 and returns the first element.
>> 
>> If the output of seq_show() for this element exceeds the initial seq_file
>> buffer, the buffer overflows. The sequence machinery reallocates a larger
>> buffer and calls seq_start() again.
>> 
>> Since *pos is now 1, the code executes key = info->last_key. However, last_key
>> is only populated in seq_next(), which hasn't been called yet. Thus, last_key
>> remains uninitialized from its kmalloc(..., GFP_USER) allocation.
>> 
>>> +     return elem;
>>> }
>> 
>> [ ... ]
>> 
>>> static int bpf_iter_init_rhash_map(void *priv_data, struct bpf_iter_aux_info *aux)
>>> {
>>> +     struct bpf_iter_seq_rhash_map_info *info = priv_data;
>>> +     struct bpf_map *map = aux->map;
>>> +
>>> +     info->last_key = kmalloc(map->key_size, GFP_USER);
>> 
>> [Severity: Medium]
>> Does this need __GFP_NOWARN?
>> 
>> Since BPF hash map key sizes can be extremely large, requesting large
>> contiguous memory blocks without __GFP_NOWARN can fail loudly under memory
>> fragmentation and trigger high-order page allocation warnings.
>> 
>>> +     if (!info->last_key)
>>> +             return -ENOMEM;
>>> +
>>> +     bpf_map_inc_with_uref(map);
>>> +     info->map = map;
>>> +     info->rhtab = container_of(map, struct bpf_rhtab, map);
>>> +     info->iter_active = false;
>>>      return 0;
>>> }
>> 
>> --
>> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
>>