Re: [PATCH bpf-next v10 1/2] bpftool: Use batch lookups for map dumps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] bpftool: silent map dump truncation on RHASH in
  dump_map_batch()
--

commit 7f62f4c69f650b2c84d0fbb5439eadc6c97b8aea
Author: Tianyi Chen <hi@tychen.cc>

bpftool: Use batch lookups for map dumps

This commit updates map_dump() to use BPF_MAP_LOOKUP_BATCH in 1 MiB batches
and fall back to bpf_map_get_next_key() after the last printed key if a
batch lookup fails.

Link: https://github.com/libbpf/bpftool/issues/63

> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 4b65edf74859b..c5c56323f0623 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
[ ... ]
> @@ -763,10 +761,75 @@ static int dump_map_elem(int fd, void *key, void *value,
>  	} else {
>  		print_entry_plain(map_info, key, value, cpu_ids, cpu_cnt);
>  	}
> +}
>
> +static int dump_map_elem(int fd, void *key, void *value,
> +			 struct bpf_map_info *map_info, struct btf *btf,
> +			 json_writer_t *btf_wtr, const int *cpu_ids, int cpu_cnt)
> +{
> +	if (bpf_map_lookup_elem(fd, key, value)) {
> +		print_entry_error(map_info, key, errno);
> +		return -1;
> +	}
> +
> +	print_map_elem(key, value, map_info, btf, btf_wtr, cpu_ids, cpu_cnt);
>  	return 0;
>  }
>
> +#define MAP_DUMP_BATCH_BYTES (1U << 20)
> +
> +/*
> + * Print the elements returned by batched lookups, leaving the last one in
> + * @key. Return true at the end of the map. After any error, the caller
> + * continues with bpf_map_get_next_key(), which walks the map in the same order.
> + * The caller must ensure that key_size + value_size is non-zero.
> + */
> +static bool dump_map_batch(int fd, void *key, void *value,
> +			   struct bpf_map_info *info, struct btf *btf,
> +			   json_writer_t *wtr, const int *cpu_ids, int cpu_cnt,
> +			   unsigned int *num_elems)
> +{
> +	size_t value_size = map_value_size(info);
> +	void *keys, *values, *batch, *in_batch = NULL;
> +	__u32 capacity, count, i;
> +	bool done = false;
> +	int err;
> +
> +	capacity = min(MAP_DUMP_BATCH_BYTES / (info->key_size + value_size),
> +		       (size_t)info->max_entries);
> +	if (!capacity)
> +		return false;
> +
> +	/* Hash table batch cursors are u32 bucket indices; other maps use keys. */
> +	batch = malloc(max(info->key_size, 4U));
> +	keys = malloc((size_t)capacity * info->key_size);
> +	values = malloc((size_t)capacity * value_size);
> +	if (!batch || !keys || !values)
> +		goto out;
> +
> +	do {
> +		count = capacity;
> +		err = bpf_map_lookup_batch(fd, in_batch, batch, keys, values,
> +					   &count, NULL);
> +		if (err && err != -ENOENT)
> +			goto out;
> +		for (i = 0; i < count; i++) {
> +			/* Preserve alignment for odd-sized BTF keys and values. */
> +			memcpy(key, keys + (size_t)i * info->key_size, info->key_size);
> +			memcpy(value, values + (size_t)i * value_size, value_size);
> +			print_map_elem(key, value, info, btf, wtr, cpu_ids, cpu_cnt);
> +		}
> +		*num_elems += count;
> +		in_batch = batch;
> +	} while (!err);
> +	done = true;

[Severity: Medium]
As reported by bot+bpf-ci@kernel.org on v9 [1], can dump_map_batch()
silently truncate a BPF_MAP_TYPE_RHASH dump when an element is deleted
concurrently?

For BPF_MAP_TYPE_RHASH, __rhtab_map_lookup_and_delete_batch() holds only
rcu_read_lock().  If dst_key is removed between copying elem->data and
calling rhashtable_next_key(&rhtab->ht, dst_key), rhashtable_next_key()
returns ERR_PTR(-ENOENT), which leaves has_next_cursor false and causes
__rhtab_map_lookup_and_delete_batch() to return -ENOENT early:

kernel/bpf/hashtab.c:__rhtab_map_lookup_and_delete_batch() {
	...
	while (elem && !IS_ERR(elem) && total < max_count) {
		memcpy(dst_key, elem->data, key_size);
		...
		elem = rhashtable_next_key(&rhtab->ht, dst_key);
		...
	}

	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;
	}
	...
	/* No more elements after this batch. */
	if (!has_next_cursor)
		ret = -ENOENT;
}

Because dump_map_batch() treats -ENOENT as end-of-map and returns true,
map_dump() skips the bpf_map_get_next_key() fallback loop (where
rhtab_map_get_next_key() restarts from NULL on -ENOENT) and exits with
success after printing only part of the map.

[1] https://lore.kernel.org/r/33b057d852c258c157f6aa31e45083418944efe0efaad1d72bcb5513205a67f6@mail.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794380384.git.hi@tychen.cc?part=1