Re: [PATCH bpf-next v4 1/3] bpftool: fix double close in map dump

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Medium] Missing error propagation when `bpf_map_get_info_by_fd()` fails on subsequent maps in `do_dump()`.
- [Medium] Double close of map file descriptors in `do_show_subset()`.
--

--- Patch [1]: [PATCH bpf-next v4 1/3] bpftool: fix double close in map dump ---
commit 8a5aea6bcad40d4506c71d6f8f2b151ddbe0ed20
Author: Yuan Chen <[email protected]>

bpftool: fix double close in map dump

This commit fixes a double close in map_dump() where the map file
descriptor was being closed in the error path, and then closed again
afterward by do_dump().

> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 6b9649294ca1a..e9f4dd9458cf1 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -895,7 +895,6 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
>  exit_free:
>  	free(key);
>  	free(value);
> -	close(fd);
>  	free_map_kv_btf(btf);
>  
>  	return err;

[Severity: Medium]
This is a pre-existing issue, but I noticed another double close pattern 
in tools/bpf/bpftool/map.c:do_show_subset():

tools/bpf/bpftool/map.c:do_show_subset() {
    ...
		if (json_output)
			show_map_close_json(fds[i], &info);
		else
			show_map_close_plain(fds[i], &info);

		close(fds[i]);
    ...
}

Since the show_map_close_json and show_map_close_plain functions already
unconditionally close the file descriptor, does the subsequent close(fds[i])
result in a double close?

This seems like it would also poison later errno checks exactly like the
bug this patch addresses.


[Severity: Medium]
This is a pre-existing issue, but while looking at do_dump() in the same 
file, there appears to be a missing error propagation:

tools/bpf/bpftool/map.c:do_dump() {
    ...
	for (i = 0; i < nb_fds; i++) {
		if (bpf_map_get_info_by_fd(fds[i], &info, &len)) {
			p_err("can't get map info: %s", strerror(errno));
			break;
		}
		err = map_dump(fds[i], &info, wtr, nb_fds > 1);
    ...
}

If a user executes bpftool with multiple map file descriptors, and
bpf_map_get_info_by_fd() fails on any iteration after the first, the loop
breaks without updating err.

Doesn't this incorrectly cause bpftool to exit with a success status if
the previous iteration succeeded, silently hiding the error from scripts or
users relying on the exit code?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1