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

On Fri, Aug 14, 2026 at 1:15 PM Roman Gushchin <[email protected]> wrote:
>
>
> > 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.

It's not just this patch, we've had quite a bunch of sashiko reviews
for months-old patches in the last few days. Just wanted to raise
awareness as this doesn't seem to be intentional.

But ok, let's hope this is a one-time glitch :)

>
> 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
> >>