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

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

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