LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH bpf] libbpf: Return btf_fd in libbpf__probe_raw_btf
@ 2019-05-29  8:29 Michal Rostecki
  2019-05-29  9:53 ` Sergei Shtylyov
  2019-05-29 15:35 ` Andrii Nakryiko
  0 siblings, 2 replies; 5+ messages in thread
From: Michal Rostecki @ 2019-05-29  8:29 UTC (permalink / raw)
  Cc: Michal Rostecki, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, netdev, bpf,
	linux-kernel

Function load_sk_storage_btf expects that libbpf__probe_raw_btf is
returning a btf descriptor, but before this change it was returning
an information about whether the probe was successful (0 or 1).
load_sk_storage_btf was using that value as an argument to the close
function, which was resulting in closing stdout and thus terminating the
process which used that dunction.

That bug was visible in bpftool. `bpftool feature` subcommand was always
exiting too early (because of closed stdout) and it didn't display all
requested probes. `bpftool -j feature` or `bpftool -p feature` were not
returning a valid json object.

Fixes: d7c4b3980c18 ("libbpf: detect supported kernel BTF features and sanitize BTF")
Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
---
 tools/lib/bpf/libbpf.c        | 36 +++++++++++++++++++++--------------
 tools/lib/bpf/libbpf_probes.c |  7 +------
 2 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 197b574406b3..bc2dca36bced 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1645,15 +1645,19 @@ static int bpf_object__probe_btf_func(struct bpf_object *obj)
 		/* FUNC x */                                    /* [3] */
 		BTF_TYPE_ENC(5, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 0), 2),
 	};
-	int res;
+	int btf_fd;
+	int ret;
 
-	res = libbpf__probe_raw_btf((char *)types, sizeof(types),
-				    strs, sizeof(strs));
-	if (res < 0)
-		return res;
-	if (res > 0)
+	btf_fd = libbpf__probe_raw_btf((char *)types, sizeof(types),
+				       strs, sizeof(strs));
+	if (btf_fd < 0)
+		ret = 0;
+	else {
+		ret = 1;
 		obj->caps.btf_func = 1;
-	return 0;
+	}
+	close(btf_fd);
+	return ret;
 }
 
 static int bpf_object__probe_btf_datasec(struct bpf_object *obj)
@@ -1670,15 +1674,19 @@ static int bpf_object__probe_btf_datasec(struct bpf_object *obj)
 		BTF_TYPE_ENC(3, BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 1), 4),
 		BTF_VAR_SECINFO_ENC(2, 0, 4),
 	};
-	int res;
+	int btf_fd;
+	int ret;
 
-	res = libbpf__probe_raw_btf((char *)types, sizeof(types),
-				    strs, sizeof(strs));
-	if (res < 0)
-		return res;
-	if (res > 0)
+	btf_fd = libbpf__probe_raw_btf((char *)types, sizeof(types),
+				       strs, sizeof(strs));
+	if (btf_fd < 0)
+		ret = 0;
+	else {
+		ret = 1;
 		obj->caps.btf_datasec = 1;
-	return 0;
+	}
+	close(btf_fd);
+	return ret;
 }
 
 static int
diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
index 5e2aa83f637a..2c2828345514 100644
--- a/tools/lib/bpf/libbpf_probes.c
+++ b/tools/lib/bpf/libbpf_probes.c
@@ -157,14 +157,9 @@ int libbpf__probe_raw_btf(const char *raw_types, size_t types_len,
 	memcpy(raw_btf + hdr.hdr_len + hdr.type_len, str_sec, hdr.str_len);
 
 	btf_fd = bpf_load_btf(raw_btf, btf_len, NULL, 0, false);
-	if (btf_fd < 0) {
-		free(raw_btf);
-		return 0;
-	}
 
-	close(btf_fd);
 	free(raw_btf);
-	return 1;
+	return btf_fd;
 }
 
 static int load_sk_storage_btf(void)
-- 
2.21.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf] libbpf: Return btf_fd in libbpf__probe_raw_btf
  2019-05-29  8:29 [PATCH bpf] libbpf: Return btf_fd in libbpf__probe_raw_btf Michal Rostecki
@ 2019-05-29  9:53 ` Sergei Shtylyov
  2019-05-29 12:59   ` Michal Rostecki
  2019-05-29 15:35 ` Andrii Nakryiko
  1 sibling, 1 reply; 5+ messages in thread
From: Sergei Shtylyov @ 2019-05-29  9:53 UTC (permalink / raw)
  To: Michal Rostecki
  Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, netdev, bpf, linux-kernel

Hello!

On 29.05.2019 11:29, Michal Rostecki wrote:

> Function load_sk_storage_btf expects that libbpf__probe_raw_btf is
> returning a btf descriptor, but before this change it was returning
> an information about whether the probe was successful (0 or 1).
> load_sk_storage_btf was using that value as an argument to the close
> function, which was resulting in closing stdout and thus terminating the
> process which used that dunction.

    Function? :-)

> That bug was visible in bpftool. `bpftool feature` subcommand was always
> exiting too early (because of closed stdout) and it didn't display all
> requested probes. `bpftool -j feature` or `bpftool -p feature` were not
> returning a valid json object.
> 
> Fixes: d7c4b3980c18 ("libbpf: detect supported kernel BTF features and sanitize BTF")
> Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf] libbpf: Return btf_fd in libbpf__probe_raw_btf
  2019-05-29  9:53 ` Sergei Shtylyov
@ 2019-05-29 12:59   ` Michal Rostecki
  0 siblings, 0 replies; 5+ messages in thread
From: Michal Rostecki @ 2019-05-29 12:59 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: Michal Rostecki, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, netdev, bpf,
	linux-kernel

On Wed, May 29, 2019 at 12:53:42PM +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 29.05.2019 11:29, Michal Rostecki wrote:
> 
> > Function load_sk_storage_btf expects that libbpf__probe_raw_btf is
> > returning a btf descriptor, but before this change it was returning
> > an information about whether the probe was successful (0 or 1).
> > load_sk_storage_btf was using that value as an argument to the close
> > function, which was resulting in closing stdout and thus terminating the
> > process which used that dunction.
> 
>    Function? :-)

Opps! I will fix in v2. Thanks!

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf] libbpf: Return btf_fd in libbpf__probe_raw_btf
  2019-05-29  8:29 [PATCH bpf] libbpf: Return btf_fd in libbpf__probe_raw_btf Michal Rostecki
  2019-05-29  9:53 ` Sergei Shtylyov
@ 2019-05-29 15:35 ` Andrii Nakryiko
  2019-05-29 15:48   ` Michal Rostecki
  1 sibling, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2019-05-29 15:35 UTC (permalink / raw)
  To: Michal Rostecki
  Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Networking, bpf, linux-kernel

On Wed, May 29, 2019 at 1:30 AM Michal Rostecki <mrostecki@opensuse.org> wrote:
>
> Function load_sk_storage_btf expects that libbpf__probe_raw_btf is
> returning a btf descriptor, but before this change it was returning
> an information about whether the probe was successful (0 or 1).
> load_sk_storage_btf was using that value as an argument to the close
> function, which was resulting in closing stdout and thus terminating the
> process which used that dunction.
>
> That bug was visible in bpftool. `bpftool feature` subcommand was always
> exiting too early (because of closed stdout) and it didn't display all
> requested probes. `bpftool -j feature` or `bpftool -p feature` were not
> returning a valid json object.
>

Thanks for the fix!

> Fixes: d7c4b3980c18 ("libbpf: detect supported kernel BTF features and sanitize BTF")
> Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
> ---
>  tools/lib/bpf/libbpf.c        | 36 +++++++++++++++++++++--------------
>  tools/lib/bpf/libbpf_probes.c |  7 +------
>  2 files changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 197b574406b3..bc2dca36bced 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1645,15 +1645,19 @@ static int bpf_object__probe_btf_func(struct bpf_object *obj)
>                 /* FUNC x */                                    /* [3] */
>                 BTF_TYPE_ENC(5, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 0), 2),
>         };
> -       int res;
> +       int btf_fd;
> +       int ret;
>
> -       res = libbpf__probe_raw_btf((char *)types, sizeof(types),
> -                                   strs, sizeof(strs));
> -       if (res < 0)
> -               return res;
> -       if (res > 0)
> +       btf_fd = libbpf__probe_raw_btf((char *)types, sizeof(types),
> +                                      strs, sizeof(strs));
> +       if (btf_fd < 0)
> +               ret = 0;
> +       else {
> +               ret = 1;

This whole ret variable seems unnecessary. Also if btf_fd is invalid,
we probably shouldn't close it. So just this should work:

btf_fd = libbpf__probe_raw_btf(...);
if (btf_fd >= 0) {
    obj->caps.btf_func = 1;
    close(btf_fd);
}
return btf_fd >= 0;

>                 obj->caps.btf_func = 1;
> -       return 0;
> +       }
> +       close(btf_fd);
> +       return ret;
>  }
>
>  static int bpf_object__probe_btf_datasec(struct bpf_object *obj)
> @@ -1670,15 +1674,19 @@ static int bpf_object__probe_btf_datasec(struct bpf_object *obj)
>                 BTF_TYPE_ENC(3, BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 1), 4),
>                 BTF_VAR_SECINFO_ENC(2, 0, 4),
>         };
> -       int res;
> +       int btf_fd;
> +       int ret;
>
> -       res = libbpf__probe_raw_btf((char *)types, sizeof(types),
> -                                   strs, sizeof(strs));
> -       if (res < 0)
> -               return res;
> -       if (res > 0)
> +       btf_fd = libbpf__probe_raw_btf((char *)types, sizeof(types),
> +                                      strs, sizeof(strs));
> +       if (btf_fd < 0)
> +               ret = 0;
> +       else {
> +               ret = 1;
>                 obj->caps.btf_datasec = 1;
> -       return 0;
> +       }
> +       close(btf_fd);

Same as above.

> +       return ret;
>  }
>
>  static int
> diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
> index 5e2aa83f637a..2c2828345514 100644
> --- a/tools/lib/bpf/libbpf_probes.c
> +++ b/tools/lib/bpf/libbpf_probes.c
> @@ -157,14 +157,9 @@ int libbpf__probe_raw_btf(const char *raw_types, size_t types_len,

I'm wondering if it's better to rename this function to something like
libbpf__load_raw_btf? probe (at least to me) implies true/false
result, so feels like it might be easily misused.

>         memcpy(raw_btf + hdr.hdr_len + hdr.type_len, str_sec, hdr.str_len);
>
>         btf_fd = bpf_load_btf(raw_btf, btf_len, NULL, 0, false);
> -       if (btf_fd < 0) {
> -               free(raw_btf);
> -               return 0;
> -       }
>
> -       close(btf_fd);
>         free(raw_btf);
> -       return 1;
> +       return btf_fd;
>  }
>
>  static int load_sk_storage_btf(void)
> --
> 2.21.0
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf] libbpf: Return btf_fd in libbpf__probe_raw_btf
  2019-05-29 15:35 ` Andrii Nakryiko
@ 2019-05-29 15:48   ` Michal Rostecki
  0 siblings, 0 replies; 5+ messages in thread
From: Michal Rostecki @ 2019-05-29 15:48 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Michal Rostecki, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Networking, bpf,
	linux-kernel

On Wed, May 29, 2019 at 08:35:25AM -0700, Andrii Nakryiko wrote:
> On Wed, May 29, 2019 at 1:30 AM Michal Rostecki <mrostecki@opensuse.org> wrote:
> >
> > Function load_sk_storage_btf expects that libbpf__probe_raw_btf is
> > returning a btf descriptor, but before this change it was returning
> > an information about whether the probe was successful (0 or 1).
> > load_sk_storage_btf was using that value as an argument to the close
> > function, which was resulting in closing stdout and thus terminating the
> > process which used that dunction.
> >
> > That bug was visible in bpftool. `bpftool feature` subcommand was always
> > exiting too early (because of closed stdout) and it didn't display all
> > requested probes. `bpftool -j feature` or `bpftool -p feature` were not
> > returning a valid json object.
> >
> 
> Thanks for the fix!
> 
> > Fixes: d7c4b3980c18 ("libbpf: detect supported kernel BTF features and sanitize BTF")
> > Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
> > ---
> >  tools/lib/bpf/libbpf.c        | 36 +++++++++++++++++++++--------------
> >  tools/lib/bpf/libbpf_probes.c |  7 +------
> >  2 files changed, 23 insertions(+), 20 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 197b574406b3..bc2dca36bced 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -1645,15 +1645,19 @@ static int bpf_object__probe_btf_func(struct bpf_object *obj)
> >                 /* FUNC x */                                    /* [3] */
> >                 BTF_TYPE_ENC(5, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 0), 2),
> >         };
> > -       int res;
> > +       int btf_fd;
> > +       int ret;
> >
> > -       res = libbpf__probe_raw_btf((char *)types, sizeof(types),
> > -                                   strs, sizeof(strs));
> > -       if (res < 0)
> > -               return res;
> > -       if (res > 0)
> > +       btf_fd = libbpf__probe_raw_btf((char *)types, sizeof(types),
> > +                                      strs, sizeof(strs));
> > +       if (btf_fd < 0)
> > +               ret = 0;
> > +       else {
> > +               ret = 1;
> 
> This whole ret variable seems unnecessary. Also if btf_fd is invalid,
> we probably shouldn't close it. So just this should work:
> 
> btf_fd = libbpf__probe_raw_btf(...);
> if (btf_fd >= 0) {
>     obj->caps.btf_func = 1;
>     close(btf_fd);
> }
> return btf_fd >= 0;
> 

Makes sense, I will do it in v3.

> >                 obj->caps.btf_func = 1;
> > -       return 0;
> > +       }
> > +       close(btf_fd);
> > +       return ret;
> >  }
> >
> >  static int bpf_object__probe_btf_datasec(struct bpf_object *obj)
> > @@ -1670,15 +1674,19 @@ static int bpf_object__probe_btf_datasec(struct bpf_object *obj)
> >                 BTF_TYPE_ENC(3, BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 1), 4),
> >                 BTF_VAR_SECINFO_ENC(2, 0, 4),
> >         };
> > -       int res;
> > +       int btf_fd;
> > +       int ret;
> >
> > -       res = libbpf__probe_raw_btf((char *)types, sizeof(types),
> > -                                   strs, sizeof(strs));
> > -       if (res < 0)
> > -               return res;
> > -       if (res > 0)
> > +       btf_fd = libbpf__probe_raw_btf((char *)types, sizeof(types),
> > +                                      strs, sizeof(strs));
> > +       if (btf_fd < 0)
> > +               ret = 0;
> > +       else {
> > +               ret = 1;
> >                 obj->caps.btf_datasec = 1;
> > -       return 0;
> > +       }
> > +       close(btf_fd);
> 
> Same as above.
> 
> > +       return ret;
> >  }
> >
> >  static int
> > diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
> > index 5e2aa83f637a..2c2828345514 100644
> > --- a/tools/lib/bpf/libbpf_probes.c
> > +++ b/tools/lib/bpf/libbpf_probes.c
> > @@ -157,14 +157,9 @@ int libbpf__probe_raw_btf(const char *raw_types, size_t types_len,
> 
> I'm wondering if it's better to rename this function to something like
> libbpf__load_raw_btf? probe (at least to me) implies true/false
> result, so feels like it might be easily misused.
> 

Good idea.

> >         memcpy(raw_btf + hdr.hdr_len + hdr.type_len, str_sec, hdr.str_len);
> >
> >         btf_fd = bpf_load_btf(raw_btf, btf_len, NULL, 0, false);
> > -       if (btf_fd < 0) {
> > -               free(raw_btf);
> > -               return 0;
> > -       }
> >
> > -       close(btf_fd);
> >         free(raw_btf);
> > -       return 1;
> > +       return btf_fd;
> >  }
> >
> >  static int load_sk_storage_btf(void)
> > --
> > 2.21.0
> >

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-05-29 15:48 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-29  8:29 [PATCH bpf] libbpf: Return btf_fd in libbpf__probe_raw_btf Michal Rostecki
2019-05-29  9:53 ` Sergei Shtylyov
2019-05-29 12:59   ` Michal Rostecki
2019-05-29 15:35 ` Andrii Nakryiko
2019-05-29 15:48   ` Michal Rostecki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).