Re: [PATCH bpf-next 1/2] bpftool: Add recursive map dumping

[bot+bpf-ci]

> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 684a8fb72414..853f775b7d52 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c

[ ... ]

> @@ -745,8 +745,10 @@ static int dump_map_elem(int fd, void *key, void *value,
>  			 json_writer_t *btf_wtr)
>  {
>  	if (bpf_map_lookup_elem(fd, key, value)) {
> -		print_entry_error(map_info, key, errno);
> -		return -1;
> +		int lookup_errno = errno;
> +
> +		print_entry_error(map_info, key, lookup_errno);
> +		return -lookup_errno;
>  	}
>
>  	if (json_output) {

[ ... ]

> @@ -874,10 +928,22 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
>  		if (err) {
>  			if (errno == ENOENT)
>  				err = 0;
> +			else if (ctx)
> +				p_err("can't get next key for map id %u: %s",
> +				      info->id, strerror(errno));
>  			break;
>  		}
> -		if (!dump_map_elem(fd, key, value, info, btf, wtr))
> +		err = dump_map_elem(fd, key, value, info, btf, wtr);
> +		if (!err) {
>  			num_elems++;
> +			if (ctx && map_is_map_of_maps(info->type)) {
> +				err = collect_inner_map(ctx, *(__u32 *)value);
> +				if (err)
> +					break;
> +			}
> +		} else if (ctx && err != -ENOENT) {
> +			break;
> +		}
>  		prev_key = key;
>  	}

Does the 'else if (ctx && err != -ENOENT) break;' path handle all
expected lookup failures correctly?

dump_map_elem() now returns -lookup_errno instead of -1, so any
bpf_map_lookup_elem() failure other than ENOENT aborts the recursive
dump. Several map types fail every syscall lookup by design:

  * BPF_MAP_TYPE_PERF_EVENT_ARRAY / BPF_MAP_TYPE_CGROUP_ARRAY:
    bpf_fd_array_map_lookup_elem() returns -ENOTSUPP when .map_fd_sys_lookup_elem
    is not defined, and perf_event_array_map_ops does not define one.
  * BPF_MAP_TYPE_XSKMAP: xsk_map_lookup_elem_sys_only() returns -EOPNOTSUPP
    unconditionally.
  * BPF_MAP_TYPE_REUSEPORT_SOCKARRAY with value_size != 8:
    bpf_fd_reuseport_array_lookup_elem() returns -ENOSPC for every key.

print_entry_error() handles all of these, emitting '<cannot read>' or
strerror(lookup_errno), and the pre-patch loop ignored the result. So
'bpftool map dump id N' on such a map prints one error marker per entry
and exits 0.

With -r, 'bpftool map dump -r id N' on any of the map types above now
fails outright and exits nonzero where the same command without -r
succeeds. This also defeats the feature's own purpose: a PERF_EVENT_ARRAY
is a legal inner map, so recursing into a HASH_OF_MAPS/ARRAY_OF_MAPS of
PERF_EVENT_ARRAYs aborts with 'Unknown error 524' on the first inner-map
entry instead of dumping the traversal.

The new fatal handling looks intended for inner-map collection failures,
which is already handled by 'if (err) break;' on the collect_inner_map()
branch above. Could the per-entry value lookup failure keep the
pre-existing tolerant behaviour and continue rather than break?

> @@ -902,6 +970,7 @@ static int do_dump(int argc, char **argv)
>  {
>  	json_writer_t *wtr = NULL, *btf_wtr = NULL;
>  	struct bpf_map_info info = {};
> +	struct map_dump_ctx ctx = {};
>  	int nb_fds, i = 0;
>  	__u32 len = sizeof(info);
>  	int *fds = NULL;

[ ... ]

> @@ -936,7 +1032,7 @@ static int do_dump(int argc, char **argv)
>  		}
>  	}
>
> -	if (wtr && nb_fds > 1)
> +	if (wtr && (nb_fds > 1 || recursive))
>  		jsonw_start_array(wtr);	/* root array */
>  	for (i = 0; i < nb_fds; i++) {
>  		if (bpf_map_get_info_by_fd(fds[i], &info, &len)) {
> @@ -944,22 +1040,28 @@ static int do_dump(int argc, char **argv)
>  			err = -1;
>  			break;
>  		}
> -		err = map_dump(fds[i], &info, wtr, nb_fds > 1);
> +		err = map_dump(fds[i], &info, wtr, nb_fds > 1 || recursive,
> +			       recursive ? &ctx : NULL);
>  		if (!wtr && i != nb_fds - 1)
>  			printf("\n");
>
>  		if (err)
>  			break;
> -		close(fds[i]);
> +		/* Keep discovered maps alive until the recursive dump is complete. */
> +		if (!recursive)
> +			close(fds[i]);
>  	}

Could the fd accumulation hit RLIMIT_NOFILE?

Under -r, every discovered inner map fd is held open until the entire
dump completes, so the number of simultaneously open fds scales with the
number of distinct maps reachable from the selected roots.

collect_inner_map() appends one fd per newly seen id and do_dump() skips
the in-loop close(), deferring all closes to exit_close. bpftool never
raises RLIMIT_NOFILE, so the inherited soft limit (commonly 1024) applies.
An ARRAY_OF_MAPS/HASH_OF_MAPS whose entries reference more distinct inner
maps than that limit makes bpf_map_get_fd_by_id_opts() fail with EMFILE,
producing 'can't open inner map id %u: Too many open files' and aborting
the dump after partial output has already been written.

max_entries for a map-of-maps is not bounded by the fd limit, so this is
reachable without any unusual privilege. Holding the fd does serve a
purpose - it pins the map so its id cannot be recycled between discovery
and dump. Could the number of concurrently held fds be bounded, or could
each discovered map be dumped as soon as it is found and closed then?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34040347400