Skip to content

bpftool: fix spurious batch file read error - #13427

Open
kernel-patches-daemon-bpf[bot] wants to merge 1 commit into
bpf-next_basefrom
series/1150744=>bpf-next
Open

bpftool: fix spurious batch file read error#13427
kernel-patches-daemon-bpf[bot] wants to merge 1 commit into
bpf-next_basefrom
series/1150744=>bpf-next

Conversation

@kernel-patches-daemon-bpf

Copy link
Copy Markdown

Pull request for series with
subject: bpftool: fix spurious batch file read error
version: 6
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150744

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 5e289c5
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150744
version: 6

@kernel-patches-review-bot

Copy link
Copy Markdown
commit 5fe483a49b4ac62c05801063ab21cde9eb158541
Author: Yuan Chen <chenyuan_fl@163.com>

bpftool: fix spurious batch file read error

This commit fixes a spurious batch file read error in bpftool by clearing
errno before each fgets() call, ensuring the post-loop check only sees the
outcome of the last read rather than stale errno from previously executed
commands. It also removes the now-unnecessary USE_LIBCAP errno reset.

> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index c91e1a6e1a1e..7a0c214f08a0 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -365,7 +365,11 @@ static int do_batch(int argc, char **argv)
>
>  	if (json_output)
>  		jsonw_start_array(json_wtr);
> -	while (fgets(buf, sizeof(buf), fp)) {
> +	for (;;) {
> +		errno = 0;
> +		if (!fgets(buf, sizeof(buf), fp))
> +			break;
> +

[ ... ]

>  		if (errno && errno != ENOENT) {
>  			p_err("reading batch file failed: %s", strerror(errno));
>  			err = -1;

This isn't a bug, but now that errno is cleared before each fgets() and the
post-loop check only sees the last fgets() outcome, is the errno != ENOENT
exemption still needed?

Looking at the code, fgets() and read() cannot fail with ENOENT. The
exemption was load-bearing before this change: commands like map show
(map.c:740), link show (link.c:1356), and map dump (map.c:875) all return
with errno == ENOENT, and the exemption prevented those stale values from
triggering false failures. This patch fixes that at the source by clearing
errno before each read.

Would dropping the errno != ENOENT check here (similar to how the
USE_LIBCAP reset is removed) make the intent clearer?

>
> @@ -467,16 +471,6 @@ int main(int argc, char **argv)
>
>  	setlinebuf(stdout);
>
> -#ifdef USE_LIBCAP
> -	/* Libcap < 2.63 hooks before main() to compute the number of
> -	 * capabilities of the running kernel, and doing so it calls prctl()
> -	 * which may fail and set errno to non-zero.
> -	 * Let's reset errno to make sure this does not interfere with the
> -	 * batch mode.
> -	 */
> -	errno = 0;
> -#endif
> -
>  	last_do_help = do_help;
>  	pretty_output = false;
>  	json_output = false;

This isn't a bug, but could the changelog use an example that still
reproduces on current bpf-next?

The commit message cites "map dump's EBADF from a double close" as the
example of a stale errno:

>     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.

Looking at the current code, map_dump() no longer closes the fd
(tools/bpf/bpftool/map.c:893-898 just has exit_free: free(key);
free(value); free_map_kv_btf(btf);), and do_dump() is the sole owner
(map.c:941-962). So there's no double close and no EBADF left behind.

A successful map dump actually leaves errno == ENOENT from
bpf_map_get_next_key() exhaustion (map.c:874-877), which the existing
errno != ENOENT exemption already suppressed. This means map dump isn't
actually a command that could have triggered the symptom described.



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

In-Reply-To-Subject: bpftool: fix spurious batch file read error
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32712601990

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: d83fba2
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150744
version: 6

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: ce36e38
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150744
version: 6

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 05ea1b6
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150744
version: 6

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 1555de3
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150744
version: 6

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 48b69cc
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150744
version: 6

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.

Clear errno before each fgets() call, so the post-loop check only
sees the outcome of the last read: zero on success or EOF, E2BIG for
an overlong line, and a genuine errno when fgets() fails.

Since errno is now reset before every read in batch mode, drop the
USE_LIBCAP errno reset in main() that existed only to keep errno clean
for the batch mode.

Fixes: 71bb428 ("tools: bpf: add bpftool")
Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants