[bpf-next,1/5] bpftool: Fix a leak of btf object
diff mbox series

Message ID 20200114224400.3027140-1-kafai@fb.com
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series
  • [bpf-next,1/5] bpftool: Fix a leak of btf object
Related show

Commit Message

Martin KaFai Lau Jan. 14, 2020, 10:44 p.m. UTC
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>
---
 tools/bpf/bpftool/map.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

Comments

Andrii Nakryiko Jan. 15, 2020, 1:10 a.m. UTC | #1
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
>
Martin KaFai Lau Jan. 15, 2020, 5:46 a.m. UTC | #2
On Tue, Jan 14, 2020 at 05:10:03PM -0800, Andrii Nakryiko wrote:
> 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?
No.  maps_have_btf() returns 1 only if all the maps have btf.

> 
> 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?
For plain_text, btf output is either for all or for none.
It is the intention of the "Fixes" patch if I read it correctly,
and it is kept as is in this bug fix.
It will become clear by doing a plain text dump on maps with and
without btf.  They are very different.

Can the output format for with and without BTF somehow merged for
plain text?  May be if it is still common to have no-BTF map
going forward but how this may look like will need another
discussion.

> 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.
The json_output is doing what you described, print BTF info
whenever available.

> 
> Maybe Paul can clarify...
> 
> 
> >         }
> >
> >         return 1;
> > -
> > -err_close:
> > -       for (; i < nb_fds; i++)
> > -               close(fds[i]);
> > -       return -1;
> >  }
> >
> >  static int
> > --
> > 2.17.1
> >
Andrii Nakryiko Jan. 15, 2020, 6:40 a.m. UTC | #3
On Tue, Jan 14, 2020 at 9:46 PM Martin Lau <kafai@fb.com> wrote:
>
> On Tue, Jan 14, 2020 at 05:10:03PM -0800, Andrii Nakryiko wrote:
> > 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?
> No.  maps_have_btf() returns 1 only if all the maps have btf.
>
> >
> > 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?
> For plain_text, btf output is either for all or for none.
> It is the intention of the "Fixes" patch if I read it correctly,
> and it is kept as is in this bug fix.
> It will become clear by doing a plain text dump on maps with and
> without btf.  They are very different.
>
> Can the output format for with and without BTF somehow merged for
> plain text?  May be if it is still common to have no-BTF map
> going forward but how this may look like will need another
> discussion.

I see, ok, seems like that behavior was intentional, I didn't mean to
start a new discussion about format :)

Acked-by: Andrii Nakryiko <andriin@fb.com>

>
> > 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.
> The json_output is doing what you described, print BTF info
> whenever available.
>
> >
> > Maybe Paul can clarify...
> >
> >
> > >         }
> > >
> > >         return 1;
> > > -
> > > -err_close:
> > > -       for (; i < nb_fds; i++)
> > > -               close(fds[i]);
> > > -       return -1;
> > >  }
> > >
> > >  static int
> > > --
> > > 2.17.1
> > >
Paul Chaignon Jan. 16, 2020, 8 a.m. UTC | #4
On Wed, Jan 15, 2020 at 6:46 AM Martin Lau <kafai@fb.com> wrote:
> On Tue, Jan 14, 2020 at 05:10:03PM -0800, Andrii Nakryiko wrote:
> > 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?
> No.  maps_have_btf() returns 1 only if all the maps have btf.
>
> >
> > 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?
> For plain_text, btf output is either for all or for none.
> It is the intention of the "Fixes" patch if I read it correctly,
> and it is kept as is in this bug fix.
> It will become clear by doing a plain text dump on maps with and
> without btf.  They are very different.

Yes, that was the intent of my patch.  As you said, I wasn't sure mixes
of BTF/non-BTF maps were common, especially in a batch sharing the same
name.

Thanks for the fixes Martin!


>
> Can the output format for with and without BTF somehow merged for
> plain text?  May be if it is still common to have no-BTF map
> going forward but how this may look like will need another
> discussion.
>
> > 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.
> The json_output is doing what you described, print BTF info
> whenever available.
>
> >
> > Maybe Paul can clarify...
> >
> >
> > >         }
> > >
> > >         return 1;
> > > -
> > > -err_close:
> > > -       for (; i < nb_fds; i++)
> > > -               close(fds[i]);
> > > -       return -1;
> > >  }
> > >
> > >  static int
> > > --
> > > 2.17.1
> > >
>

Patch
diff mbox series

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;
 	}
 
 	return 1;
-
-err_close:
-	for (; i < nb_fds; i++)
-		close(fds[i]);
-	return -1;
 }
 
 static int