Re: [PATCH bpf-next v7 05/12] bpf: Implement iteration ops for resizable hashtab

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Map iteration operations (bpf_for_each_map_elem and batch operations) prematurely and silently abort if an element is concurrently deleted or intentionally deleted by the BPF program.
- [High] Lack of atomicity in `lookup_and_delete_batch` on RHASH maps allows returning duplicate elements to concurrent consumers.
--

commit 54494d14a4d025348520c4921d8c583a6d4257a2
Author: Mykyta Yatsenko <yatsenko@meta.com>

bpf: Implement iteration ops for resizable hashtab
    
Implement get_next_key, batch lookup/lookup-and-delete, for_each_map_elem
callback, and the seq_file BPF iterator for BPF_MAP_TYPE_RHASH.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c

[ ... ]

> @@ -3020,8 +3020,79 @@ static void rhtab_map_free_internal_structs(struct bpf_map *map)
>  static long bpf_each_rhash_elem(struct bpf_map *map, bpf_callback_t callback_fn,
>  				void *callback_ctx, u64 flags)
>  {
> +	struct bpf_rhtab *rhtab = container_of(map, struct bpf_rhtab, map);
> +	void *prev_key = NULL;
> +	struct rhtab_elem *elem;
> +	int num_elems = 0;
> +	u64 ret = 0;

[ ... ]

> +	rcu_read_lock();
> +	/*
> +	 * Best-effort iteration: if rhashtable is concurrently resized or
> +	 * elements are deleted/inserted, there may be missed or duplicate
> +	 * elements visited.
> +	 */
> +	while ((elem = rhashtable_next_key(&rhtab->ht, prev_key))) {
> +		if (IS_ERR(elem))
> +			break;

[Severity: High]
If the BPF callback intentionally deletes the current element (which is a
common pattern for map cleanup), will rhashtable_next_key() return -ENOENT
because prev_key is no longer in the table?

If so, does this loop prematurely break and return num_elems without an
error code, silently truncating the iteration?

> +		num_elems++;
> +		ret = callback_fn((u64)(long)map,
> +				  (u64)(long)elem->data,
> +				  (u64)(long)rhtab_elem_value(elem, map->key_size),
> +				  (u64)(long)callback_ctx, 0);
> +		if (ret)
> +			break;
> +
> +		prev_key = elem->data;	/* valid while RCU held */
> +	}
> +	rcu_read_unlock();
> +
> +	return num_elems;
>  }

[ ... ]

> +static int __rhtab_map_lookup_and_delete_batch(struct bpf_map *map,
> +					       const union bpf_attr *attr,
> +					       union bpf_attr __user *uattr,
> +					       bool do_delete)
> +{
> +	struct bpf_rhtab *rhtab = container_of(map, struct bpf_rhtab, map);

[ ... ]

> +	while (elem && !IS_ERR(elem) && total < max_count) {
> +		memcpy(dst_key, elem->data, key_size);
> +		rhtab_read_elem_value(map, dst_val, elem, elem_map_flags);
> +		check_and_init_map_value(map, dst_val);
> +
> +		if (do_delete)
> +			del_elems[total] = elem;
> +
> +		elem = rhashtable_next_key(&rhtab->ht, dst_key);

[Severity: High]
If a concurrent thread deletes the dst_key element, will
rhashtable_next_key() return -ENOENT and break the loop?

If this happens, has_next_cursor remains false and the function returns
-ENOENT to userspace. Will userspace misinterpret this -ENOENT as the
legitimate end of the map and stop iterating, dropping all remaining
unvisited elements?

> +		dst_key += key_size;
> +		dst_val += value_size;
> +		total++;
> +
> +		/* Bail to userspace to avoid stalls. */
> +		if (need_resched())
> +			break;
> +	}
> +
> +	if (elem && !IS_ERR(elem)) {
> +		/* Stash next-to-process key as cursor for the next batch. */
> +		memcpy(cursor, elem->data, key_size);
> +		has_next_cursor = true;
> +	}
> +
> +	if (do_delete) {
> +		for (i = 0; i < total; i++)
> +			rhtab_delete_elem(rhtab, del_elems[i], NULL, 0);
> +	}

[Severity: High]
Because this function iterates locklessly under RCU and defers deletion
until after the loop, can concurrent batch calls overlap their reads and
observe the exact same elements?

If they both copy the identical elements into their userspace buffers, only
one will successfully delete the element, but the return value of
rhtab_delete_elem() is ignored. Could both threads successfully return the
same elements to userspace, violating the atomic pop semantics required by
applications draining the queue?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-rhash-v7-0-5b8e05f8630d@meta.com?part=5