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

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