Netdev Archive on lore.kernel.org
 help / color / Atom feed
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
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
>

  reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-14 22:43 [PATCH bpf-next 0/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id Martin KaFai Lau
2020-01-14 22:44 ` [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object Martin KaFai Lau
2020-01-15  1:10   ` Andrii Nakryiko [this message]
2020-01-15  5:46     ` Martin Lau
2020-01-15  6:40       ` Andrii Nakryiko
2020-01-16  8:00       ` Paul Chaignon
2020-01-14 22:44 ` [PATCH bpf-next 2/5] bpftool: Fix missing BTF output for json during map dump Martin KaFai Lau
2020-01-15  1:34   ` Andrii Nakryiko
2020-01-15  5:54     ` Martin Lau
2020-01-15  6:36       ` Andrii Nakryiko
2020-01-14 22:44 ` [PATCH bpf-next 3/5] libbpf: Expose bpf_find_kernel_btf to libbpf_internal.h Martin KaFai Lau
2020-01-15  1:43   ` Andrii Nakryiko
2020-01-15  5:56     ` Martin Lau
2020-01-14 22:44 ` [PATCH bpf-next 4/5] bpftool: Add struct_ops map name Martin KaFai Lau
2020-01-15  1:45   ` Andrii Nakryiko
2020-01-14 22:44 ` [PATCH bpf-next 5/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id Martin KaFai Lau
2020-01-15  1:49   ` Andrii Nakryiko
2020-01-15  6:04     ` Martin Lau
2020-01-15  6:39       ` Andrii Nakryiko

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEf4BzZd-NmpJqYStpDTSAFmN=EDCLftqoYBaSAKECOY8ooR_w@mail.gmail.com' \
    --to=andrii.nakryiko@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=kafai@fb.com \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    --cc=paul.chaignon@orange.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git