[PATCH bpf-next 0/2] bpftool: fix double close in map dump and batch file error handling

[PATCH bpf-next 0/2] bpftool: fix double close in map dump and batch file error handling

[chenyuan_fl]

From: Yuan Chen <chenyuan@kylinos.cn>

bpftool's map dump closes the map fd twice: map_dump() closes it on both
its success and error paths, and do_dump() closes the same fd again.
This leaves errno set to EBADF after a dump.

The batch subcommand then misinterprets that stale errno: do_batch()
checks errno after reading the batch file to detect I/O failures, but
fgets() does not clear errno on success, so a leftover EBADF makes
bpftool report a spurious "reading batch file failed" error and exit
non-zero even though every command succeeded.

Patch 1 makes do_dump() the sole owner of the map fd by removing the
close from map_dump(), eliminating the double close and the EBADF source.
Patch 2 makes do_batch() use ferror() to detect genuine read failures
and tracks the too-long-line case explicitly, so stale errno can no
longer trigger a spurious error from any other source either.

Yuan Chen (2):
  bpftool: fix double close in map dump
  bpftool: fix spurious batch file read error

 tools/bpf/bpftool/main.c | 8 ++++++--
 tools/bpf/bpftool/map.c  | 1 -
 2 files changed, 6 insertions(+), 3 deletions(-)

-- 
2.54.0

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

[chenyuan_fl]

From: Yuan Chen <chenyuan@kylinos.cn>

map_dump() closes the map fd in its error path, and do_dump() then
closes the same fd again after a successful dump. Closing an already
closed fd leaves errno set to EBADF, which poisons later errno checks
such as the batch file read check in do_batch(). Let do_dump() own the
fd and remove the close from map_dump().

Fixes: 99f9863a0c45f ("bpftool: Match maps by name")
Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
---
 tools/bpf/bpftool/map.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 6b9649294ca1..e9f4dd9458cf 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;
-- 
2.54.0

[PATCH bpf-next 2/2] bpftool: fix spurious batch file read error

[chenyuan_fl]

From: Yuan Chen <chenyuan@kylinos.cn>

do_batch() checks errno after the read loop to detect read failures,
but fgets() does not clear errno on success, so a stale errno left by
a previously executed command (e.g. map dump's EBADF from a double
close) makes bpftool report a batch file read failure and exit with an
error even though every command succeeded. Use ferror() instead, and
track the too-long-line case explicitly.

Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool")
Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
---
 tools/bpf/bpftool/main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index c91e1a6e1a1e..c9a86039c400 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -336,6 +336,7 @@ static int do_batch(int argc, char **argv)
 	char buf[BATCH_LINE_LEN_MAX], contline[BATCH_LINE_LEN_MAX];
 	char *n_argv[BATCH_ARG_NB_MAX];
 	unsigned int lines = 0;
+	bool line_too_long = false;
 	int n_argc;
 	FILE *fp;
 	char *cp;
@@ -371,7 +372,7 @@ static int do_batch(int argc, char **argv)
 			*cp = '\0';
 
 		if (strlen(buf) == sizeof(buf) - 1) {
-			errno = E2BIG;
+			line_too_long = true;
 			break;
 		}
 
@@ -429,7 +430,10 @@ static int do_batch(int argc, char **argv)
 		lines++;
 	}
 
-	if (errno && errno != ENOENT) {
+	if (line_too_long) {
+		p_err("reading batch file failed: %s", strerror(E2BIG));
+		err = -1;
+	} else if (ferror(fp)) {
 		p_err("reading batch file failed: %s", strerror(errno));
 		err = -1;
 	} else {
-- 
2.54.0

Re: [PATCH bpf-next 2/2] bpftool: fix spurious batch file read error

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the comment stripping logic bypasses the line truncation check, allowing portions of excessively long commented lines to be silently executed as separate commands.
--

--- Patch [2]: [PATCH bpf-next 2/2] bpftool: fix spurious batch file read error ---
commit 858e1807e2fde336612fcf607106fe8e9010fc61
Author: Yuan Chen <chenyuan@kylinos.cn>

bpftool: fix spurious batch file read error

This commit addresses a bug where do_batch() incorrectly detects a read
failure due to a stale errno from a previously executed command. It replaces
the errno check with ferror(fp) and explicitly tracks cases where a line
exceeds the buffer size limit.

> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index c91e1a6e1a1ec..c9a86039c400b 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
[ ... ]
> @@ -371,7 +372,7 @@ static int do_batch(int argc, char **argv)
>  			*cp = '\0';
>  
>  		if (strlen(buf) == sizeof(buf) - 1) {
> -			errno = E2BIG;
> +			line_too_long = true;

[Severity: High]
This isn't a bug introduced by this patch, but doesn't the comment stripping
logic right above this in do_batch() bypass the line truncation check?

If a batch file line exceeds the buffer limit (4095 characters) and contains
a '#' character in the first 4095 bytes, fgets() reads the first 4095 bytes,
but the '#' causes the string to be truncated early by setting '*cp = '\0''. 
This means strlen(buf) becomes less than sizeof(buf) - 1, incorrectly bypassing
this check.

Could this allow the unread remainder of an excessively long commented line to
remain in the file stream, where it would be parsed and executed as a separate
command on the next loop iteration?

>  			break;
>  		}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810015216.2369940-1-chenyuan_fl@163.com?part=2