Re: [PATCH bpf-next] bpftool: Clear errno after libcap's checks

From: Daniel Borkmann <daniel@iogearbox.net>
To: Quentin Monnet <quentin@isovalent.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>
Cc: Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@kernel.org>,
	Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>,
	Jiri Olsa <jolsa@kernel.org>,
	bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next] bpftool: Clear errno after libcap's checks
Date: Mon, 15 Aug 2022 17:33:14 +0200	[thread overview]
Message-ID: <dfda4b3e-2935-fd19-ce62-f331c07d6921@iogearbox.net> (raw)
In-Reply-To: <20220812153727.224500-2-quentin@isovalent.com>

On 8/12/22 5:37 PM, Quentin Monnet wrote:
> When bpftool is linked against libcap, the library runs a "constructor"
> function to compute the number of capabilities of the running kernel
> [0], at the beginning of the execution of the program. As part of this,
> it performs multiple calls to prctl(). Some of these may fail, and set
> errno to a non-zero value:
> 
>      # strace -e prctl ./bpftool version
>      prctl(PR_CAPBSET_READ, CAP_MAC_OVERRIDE) = 1
>      prctl(PR_CAPBSET_READ, 0x30 /* CAP_??? */) = -1 EINVAL (Invalid argument)
>      prctl(PR_CAPBSET_READ, CAP_CHECKPOINT_RESTORE) = 1
>      prctl(PR_CAPBSET_READ, 0x2c /* CAP_??? */) = -1 EINVAL (Invalid argument)
>      prctl(PR_CAPBSET_READ, 0x2a /* CAP_??? */) = -1 EINVAL (Invalid argument)
>      prctl(PR_CAPBSET_READ, 0x29 /* CAP_??? */) = -1 EINVAL (Invalid argument)
>      ** fprintf added at the top of main(): we have errno == 1
>      ./bpftool v7.0.0
>      using libbpf v1.0
>      features: libbfd, libbpf_strict, skeletons
>      +++ exited with 0 +++
> 
> Let's clean errno at the beginning of the main() function, to make sure
> that these checks do not interfere with the batch mode, where we error
> out if errno is set after a bpftool command.
> 
> [0] https://git.kernel.org/pub/scm/libs/libcap/libcap.git/tree/libcap/cap_alloc.c?h=v1.2.65#n20
> 
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
>   tools/bpf/bpftool/main.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 451cefc2d0da..c0e2e4fedbe8 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -435,6 +435,9 @@ int main(int argc, char **argv)
>   
>   	setlinebuf(stdout);
>   
> +	/* Libcap */

Good catch! The comment is a bit too terse, could you improve it, so that it's
clear from reading code (w/o digging through git log) why we need to reset errno
in this location? Thx

> +	errno = 0;
> +
>   	last_do_help = do_help;
>   	pretty_output = false;
>   	json_output = false;
> 