Re: [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Martin KaFai Lau <kafai@fb.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	David Miller <davem@davemloft.net>,
	Kernel Team <kernel-team@fb.com>,
	Networking <netdev@vger.kernel.org>,
	Paul Chaignon <paul.chaignon@orange.com>
Subject: Re: [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object
Date: Tue, 14 Jan 2020 17:10:03 -0800	[thread overview]
Message-ID: <CAEf4BzZd-NmpJqYStpDTSAFmN=EDCLftqoYBaSAKECOY8ooR_w@mail.gmail.com> (raw)
In-Reply-To: <20200114224400.3027140-1-kafai@fb.com>

On Tue, Jan 14, 2020 at 2:44 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> When testing a map has btf or not, maps_have_btf() tests it by actually
> getting a btf_fd from sys_bpf(BPF_BTF_GET_FD_BY_ID). However, it
> forgot to btf__free() it.
>
> In maps_have_btf() stage, there is no need to test it by really
> calling sys_bpf(BPF_BTF_GET_FD_BY_ID). Testing non zero
> info.btf_id is good enough.
>
> Also, the err_close case is unnecessary, and also causes double
> close() because the calling func do_dump() will close() all fds again.
>
> Fixes: 99f9863a0c45 ("bpftool: Match maps by name")
> Cc: Paul Chaignon <paul.chaignon@orange.com>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---

this is clearly a simplification, but isn't do_dump still buggy? see below

>  tools/bpf/bpftool/map.c | 16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index c01f76fa6876..e00e9e19d6b7 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -915,32 +915,20 @@ static int maps_have_btf(int *fds, int nb_fds)
>  {
>         struct bpf_map_info info = {};
>         __u32 len = sizeof(info);
> -       struct btf *btf = NULL;
>         int err, i;
>
>         for (i = 0; i < nb_fds; i++) {
>                 err = bpf_obj_get_info_by_fd(fds[i], &info, &len);
>                 if (err) {
>                         p_err("can't get map info: %s", strerror(errno));
> -                       goto err_close;
> -               }
> -
> -               err = btf__get_from_id(info.btf_id, &btf);
> -               if (err) {
> -                       p_err("failed to get btf");
> -                       goto err_close;
> +                       return -1;
>                 }
>
> -               if (!btf)
> +               if (!info.btf_id)
>                         return 0;

if info.btf_id is non-zero, shouldn't we immediately return 1 and be
done with it?

I'm also worried about do_dump logic. What's the behavior when some
maps do have BTF and some don't? Should we use btf_writer for all,
some or none maps for that case? I'd expect we'd use BTF info for
those maps that have BTF and fall back to raw output for those that
don't, but I'm not sure that how code behaves right now.

Maybe Paul can clarify...

>         }
>
>         return 1;
> -
> -err_close:
> -       for (; i < nb_fds; i++)
> -               close(fds[i]);
> -       return -1;
>  }
>
>  static int
> --
> 2.17.1
>